Sorry. Correct some critical wording.
“ACPI NVS and 4G limitation are history reason and are NOT necessary currently.” Jeff ________________________________ 发件人: edk2-devel <[email protected]> 代表 Fan Jeff <[email protected]> 发送时间: Friday, August 10, 2018 12:00:08 PM 收件人: Dong, Eric; 'Laszlo Ersek' 抄送: Ni, Ruiyu; Kinney, Michael D; [email protected] 主题: [edk2] 答复: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation. Hi, ACPI NVS and 4G limitation are history reason and are necessary currently. I agree to do such cleanup from header file and implementation. Long term, could we think of combine CpuFeaturesDxe and CpuS3DataDxe? Jeff 发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986>应用 ________________________________ 发件人: edk2-devel <[email protected]> 代表 Dong, Eric <[email protected]> 发送时间: Friday, August 10, 2018 11:39:16 AM 收件人: 'Laszlo Ersek' 抄送: Ni, Ruiyu; Kinney, Michael D; [email protected] 主题: Re: [edk2] [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation. Hi Laszlo, > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of > Laszlo Ersek > Sent: Thursday, August 9, 2018 12:55 AM > To: Dong, Eric <[email protected]> > Cc: Ni, Ruiyu <[email protected]>; Kinney, Michael D > <[email protected]>; [email protected] > Subject: Re: [edk2] [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change > Memory Type and address limitation. > > Hello Eric, > > On 08/08/18 09:40, Eric Dong wrote: > > Because CpuS3Data memory will be copy to smram at SmmReadToLock > point, > > so the memory type no need to be ACPI NVS type, also the address not > > limit to below 4G. > > This change remove the limit of ACPI NVS memory type and below 4G. > > > > Pass OS boot and resume from S3 test. > > > > Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959 > > > > Reported-by: Marvin Häuser <[email protected]> > > Suggested-by: Fan Jeff <[email protected]> > > (1) I believe these tags don't apply to this patch -- this patch seems to be a > new idea / addition. > Yes, will remove it in my next version patch. > > > Cc: Marvin Häuser <[email protected]> > > Cc: Fan Jeff <[email protected]> > > Cc: Laszlo Ersek <[email protected]> > > Cc: Ruiyu Ni <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <[email protected]> > > --- > > UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 60 > > +++++++----------------------- > -- > > UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 + > > 2 files changed, 14 insertions(+), 47 deletions(-) > > > > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > > index dccb406b8d..d8eb8c976f 100644 > > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > > @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF > ANY KIND, EITHER EXPRESS OR IMPLIED. > > #include <Library/UefiBootServicesTableLib.h> > > #include <Library/DebugLib.h> > > #include <Library/MtrrLib.h> > > +#include <Library/MemoryAllocationLib.h> > > > > #include <Protocol/MpService.h> > > #include <Guid/EventGroup.h> > > @@ -45,42 +46,6 @@ typedef struct { > > IA32_DESCRIPTOR IdtrProfile; > > } ACPI_CPU_DATA_EX; > > > > -/** > > - Allocate EfiACPIMemoryNVS below 4G memory address. > > - > > - This function allocates EfiACPIMemoryNVS below 4G memory address. > > - > > - @param[in] Size Size of memory to allocate. > > - > > - @return Allocated address for output. > > - > > -**/ > > -VOID * > > -AllocateAcpiNvsMemoryBelow4G ( > > - IN UINTN Size > > - ) > > -{ > > - EFI_PHYSICAL_ADDRESS Address; > > - EFI_STATUS Status; > > - VOID *Buffer; > > - > > - Address = BASE_4GB - 1; > > - Status = gBS->AllocatePages ( > > - AllocateMaxAddress, > > - EfiACPIMemoryNVS, > > - EFI_SIZE_TO_PAGES (Size), > > - &Address > > - ); > > - if (EFI_ERROR (Status)) { > > - return NULL; > > - } > > - > > - Buffer = (VOID *)(UINTN)Address; > > - ZeroMem (Buffer, Size); > > - > > - return Buffer; > > -} > > - > > /** > > Callback function executed when the EndOfDxe event group is signaled. > > > > @@ -150,7 +115,6 @@ CpuS3DataInitialize ( > > EFI_MP_SERVICES_PROTOCOL *MpServices; > > UINTN NumberOfCpus; > > UINTN NumberOfEnabledProcessors; > > - VOID *Stack; > > UINTN TableSize; > > CPU_REGISTER_TABLE *RegisterTable; > > UINTN Index; > > @@ -171,10 +135,7 @@ CpuS3DataInitialize ( > > // > > OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 > > (PcdCpuS3DataAddress); > > > > - // > > - // Allocate ACPI NVS memory below 4G memory for use on ACPI S3 > resume. > > - // > > - AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof > > (ACPI_CPU_DATA_EX)); > > + AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof > > + (ACPI_CPU_DATA_EX))); > > (2) In the original AllocateAcpiNvsMemoryBelow4G() function, we have a > ZeroMem() call. For compatibility, I think we should call > AllocateZeroPages() here. > > (Previously, the trailing portion of the last page may not have been zeroed, > but it's not a problem to zero more than before.) > Agree, will use AllocateZeroPages. > > (3) The "UefiCpuPkg/Include/AcpiCpuData.h" header states that > ACPI_CPU_DATA (the structure itself) "must be allocated below 4GB from > memory of type EfiACPIMemoryNVS". > > If we have determined that this is no longer necessary, then: > - we should first update the documentation in "AcpiCpuData.h" first, > - the commit message should carefully explain why the change is valid. > Agree, will do it. > In particular, my concern is not about the normal boot path. (The normal boot > path is explained for example in the message of commit 92b87f1c8c0b, > "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE", > 2015-11-30.) I agree that copying from BootServicesData type memory into > SMRAM is fine, during normal boot. > > Instead, my concern is with the S3 resume path, when the data from the > SMRAM buffer is *restored*. I seem to recall a case when the data was first > restored from SMRAM to the original AcpiNVS allocation. I searched the code base and not found any code needs to restore the original AcpiNVS memory. > If we make that > area BootServicesData now, then the OS will reuse it, and then at > S3 resume, we overwrite OS data. > > ... After reviewing the GetAcpiCpuData() function in > "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", I *think* this is a valid change, > because GetAcpiCpuData() never remembers the address of the > ACPI_CPU_DATA structure, or the addresses of its fields. > > However, I would like Mike to review this as well (CC'ing him). > > > (4) Because ACPI_CPU_DATA_EX contains MtrrTable, GdtrProfile and > IdtrProfile too, not just ACPI_CPU_DATA, the above change moves all of those > fields into BootServicesData type memory. In turn, the > EFI_PHYSICAL_ADDRESS fields in ACPI_CPU_DATA that have the same names > will point into BootServicesData type memory. > > This conflicts with the requirements that are documented in > "UefiCpuPkg/Include/AcpiCpuData.h" for all three fields: MtrrTable, > GdtrProfile and IdtrProfile. > I will add a separate patch to remove the memory type and below 4G limitation in the header file. > If we want to change the allocation of these objects (of type MTRR_SETTINGS, > IA32_DESCRIPTOR and IA32_DESCRIPTOR, respectively), then we should audit > their use during S3 resume in the PiSmmCpuDxeSmm driver, and document > them one-by-one. > > ... Based on GetAcpiCpuData() again, I *think* this is valid change, > functionally speaking, but I'd like Mike to review as well. > > > > ASSERT (AcpiCpuDataEx != NULL); > > AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData; > > > > @@ -210,11 +171,16 @@ CpuS3DataInitialize ( > > AcpiCpuData->MtrrTable = > (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable; > > > > // > > - // Allocate stack space for all CPUs > > + // Allocate stack space for all CPUs, use ACPI NVS memory type > > + because it will // not copy to smram at Smm ready to lock point. > > // > > - Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus * > > AcpiCpuData->StackSize); > > - ASSERT (Stack != NULL); > > - AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack; > > + Status = gBS->AllocatePages ( > > + AllocateAnyPages, > > + EfiACPIMemoryNVS, > > + EFI_SIZE_TO_PAGES (NumberOfCpus * AcpiCpuData- > >StackSize), > > + &AcpiCpuData->StackAddress > > + ); > > + ASSERT_EFI_ERROR (Status); > > (5) The leading comment should be clarified. We should state that during > S3 resume, the stack will only be used as scratch space, i.e. we won't read > anything from it before we write to it, in PiSmmCpuDxeSemm. > Otherwise, it would be a security bug to keep the area in AcpiNVS and not in > SMRAM. Agree, will update the comments. > > > (6) This change requires a separate investigation from the rest of the patch, > so it should be in a separate patch. > > That implies we should first change this call site, and then remove > AllocateAcpiNvsMemoryBelow4G() in a final patch. Agree, will create a separate patch for this issue. > > > (7) Why is it OK to lift the 4GB limitation? ... I think it may be valid > indeed, > because PrepareApStartupVector() stores StackAddress to "mExchangeInfo- > >StackStart" (which has type (VOID*)), and because > "UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with: > > add edi, StackStartAddressLocation > add rax, qword [edi] > mov rsp, rax > mov qword [edi], rax > > in long-mode code. But, again, this should be documented in the > (separate) patch's commit message. > > > (8) The change breaks the documentation on "ACPI_CPU_DATA.StackAddress", > in "UefiCpuPkg/Include/AcpiCpuData.h". The documentation should be > updated. > > > > > > // > > // Get the boot processor's GDT and IDT @@ -227,7 +193,7 @@ > > CpuS3DataInitialize ( > > // > > GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1; > > IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1; > > - Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize); > > + Gdt = AllocatePages (EFI_SIZE_TO_PAGES (GdtSize + IdtSize)); > > ASSERT (Gdt != NULL); > > Idt = (VOID *)((UINTN)Gdt + GdtSize); > > CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize); > > (9) This change breaks the requirements in > "UefiCpuPkg/Include/AcpiCpuData.h" that both "ACPI_CPU_DATA.GdtrProfile- > >Base" and "ACPI_CPU_DATA.IdtrProfile->Base" > point into AcpiNVS data. > > ("The buffer for GDT must also be allocated below 4GB from memory of type > EfiACPIMemoryNVS" / "The buffer for IDT must also be allocated below 4GB > from memory of type EfiACPIMemoryNVS".) > > > (10) And, indeed, this is the bug that I suspected in point (3), with regard > to > the S3 resume path. Namely: > > > Please consider the GetAcpiCpuData() function in > "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c". This function: > > (10a) copies the IDT and GDT *descriptors* into dynamically allocated > SMRAM, pointed-to by the "mAcpiCpuData.GdtrProfile" and > "mAcpiCpuData.IdtrProfile" fields; > > (10b) copies the IDT and GDT themselves into dynamically allocated SMRAM, > pointed-to by the "mGdtForAp" and "mIdtForAp" global variables, > > (10c) *does not* update the "Base" members of the IDT and GDT descriptors > that are saved in SMRAM in step (10a). Those continue to point to memory > that was allocated by CpuS3DataDxe in the > CpuS3DataInitialize() function. Agree this is a bug, will create a separate patch to fix this issue. > > > Now, consider the PrepareApStartupVector() function in > "UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c", which runs during S3 resume. > That > function: > > (10d) copies the IDT and GDT descriptors that were saved in (10a) into > "mExchangeInfo" -- this is just an SMRAM-to-SMRAM copy, however the Base > members of the descriptors point to the original CpuS3DataDxe allocation, > > (10e) the IDT and the GDT are themselves restored from step (10b), i.e. > from the "mGdtForAp" and "mIdtForAp" SMRAM global variables, to the > original CpuS3DataDxe allocation address. The comment says: > > > // > > // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI > NVS memory > > // > > In other words, while the IDT and GDT *descriptors* aren't restored in-place, > the IDT and GDT themselves are. > > This means that the above code change causes PiSmmCpuDxeSmm to > overwrite OS memory at S3 resume. > > > Continuing: > > On 08/08/18 09:40, Eric Dong wrote: > > @@ -243,7 +209,7 @@ CpuS3DataInitialize ( > > // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable > for all CPUs > > // > > TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE); > > - RegisterTable = (CPU_REGISTER_TABLE > *)AllocateAcpiNvsMemoryBelow4G (TableSize); > > + RegisterTable = AllocatePages (EFI_SIZE_TO_PAGES (TableSize)); > > ASSERT (RegisterTable != NULL); > > > > for (Index = 0; Index < NumberOfCpus; Index++) { > > (11) This looks valid, beyond breaking the documentation in > "UefiCpuPkg/Include/AcpiCpuData.h", of the members > "PreSmmInitRegisterTable" and "RegisterTable". > > > > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf > > b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf > > index 480c98ebcd..c16731529c 100644 > > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf > > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf > > @@ -51,6 +51,7 @@ > > DebugLib > > BaseLib > > MtrrLib > > + MemoryAllocationLib > > > > [Guids] > > gEfiEndOfDxeEventGroupGuid ## CONSUMES ## Event > > > > I feel uncomfortable about this patch. It is very tricky to review, and if we > make mistakes, we could corrupt OS data, or (worse) perhaps even > compromise SMRAM. > > And, all the gains that I can imagine are, "save a few 32-bit AcpiNVS pages". > I > haven't measured, but it doesn't look like an attractive trade-off to me. And, > platforms that disable S3 via "PcdAcpiS3Enable" > don't allocate this memory anyway. > > Furthermore, the memory footprint optimization does not seem related to the > CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency issue that Marvin > reported (and that is tracked in bug #959). > > I suggest that we drop this patch. We have reach the agreement that this is a "Not a correctly used" code bug, and it is caused by not do the related code clean when enable new features. So I prefer to do this clean up to make code more clean. > > Thanks, > Laszlo > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
