From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 57D1B210DF4EF for ; Wed, 8 Aug 2018 09:55:26 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E4E4583221; Wed, 8 Aug 2018 16:55:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-208.rdu2.redhat.com [10.10.120.208]) by smtp.corp.redhat.com (Postfix) with ESMTP id 01F7D1C739; Wed, 8 Aug 2018 16:55:24 +0000 (UTC) To: Eric Dong Cc: edk2-devel@lists.01.org, Ruiyu Ni , Michael Kinney References: <20180808074006.21360-1-eric.dong@intel.com> <20180808074006.21360-2-eric.dong@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 8 Aug 2018 18:55:24 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180808074006.21360-2-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 08 Aug 2018 16:55:25 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 08 Aug 2018 16:55:25 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v2 2/2] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Aug 2018 16:55:27 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 > Suggested-by: Fan Jeff (1) I believe these tags don't apply to this patch -- this patch seems to be a new idea / addition. > Cc: Marvin Häuser > Cc: Fan Jeff > Cc: Laszlo Ersek > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > 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 > #include > #include > +#include > > #include > #include > @@ -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.) (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. 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. 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. 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. (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. (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. 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. Thanks, Laszlo