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 75BD7210D97A6 for ; Fri, 10 Aug 2018 09:12:57 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0A66577741; Fri, 10 Aug 2018 16:12:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-207.rdu2.redhat.com [10.10.120.207]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8937010075C6; Fri, 10 Aug 2018 16:12:55 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: =?UTF-8?Q?Marvin_H=c3=a4user?= , Fan Jeff , Ruiyu Ni References: <20180810041909.12776-1-eric.dong@intel.com> <20180810041909.12776-4-eric.dong@intel.com> From: Laszlo Ersek Message-ID: Date: Fri, 10 Aug 2018 18:12:54 +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: <20180810041909.12776-4-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Fri, 10 Aug 2018 16:12:57 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Fri, 10 Aug 2018 16:12:57 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v3 3/5] 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: Fri, 10 Aug 2018 16:12:58 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 08/10/18 06:19, Eric Dong wrote: > Because CpuS3Data memory will be copy to smram at SmmReadyToLock point, > 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. > > 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 | 43 +++++++++++++++++++++++++++++++------ > 1 file changed, 36 insertions(+), 7 deletions(-) > > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > index dccb406b8d..5b99a6e759 100644 > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c > @@ -81,6 +81,38 @@ AllocateAcpiNvsMemoryBelow4G ( > return Buffer; > } > > +/** > + Allocate EfiBootService memory. > + > + @param[in] Size Size of memory to allocate. > + > + @return Allocated address for output. > + > +**/ > +VOID * > +AllocateBootServiceMemory ( > + IN UINTN Size > + ) > +{ > + EFI_PHYSICAL_ADDRESS Address; > + EFI_STATUS Status; > + VOID *Buffer; > + > + Status = gBS->AllocatePages ( > + AllocateAnyPages, > + EfiBootServicesData, > + EFI_SIZE_TO_PAGES (Size), > + &Address > + ); > + if (EFI_ERROR (Status)) { > + return NULL; > + } > + > + Buffer = (VOID *)(UINTN)Address; > + ZeroMem (Buffer, Size); > + > + return Buffer; > +} (1) If I remember correctly, we discussed AllocateZeroPages() for this. Why did you decide against it? CpuS3DataDxe is a DXE_DRIVER, and the matching MemoryAllocationLib instance would allocate Boot Services Data type memory. > /** > Callback function executed when the EndOfDxe event group is signaled. > > @@ -171,10 +203,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 = AllocateBootServiceMemory (sizeof (ACPI_CPU_DATA_EX)); > ASSERT (AcpiCpuDataEx != NULL); > AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData; > > @@ -223,11 +252,11 @@ CpuS3DataInitialize ( > AsmReadIdtr (&AcpiCpuDataEx->IdtrProfile); > (2) In the previous patch, we lifted the 4GB limitation on the stack address (while preserving the memory type restriction as AcpiNVS). However, you continue to allocate the stack with AllocateAcpiNvsMemoryBelow4G(). I don't think that's consistent with the purpose of this patch set, or with the documentation change in the previous patch. We should allocate the stack as AcpiNVS without address limitation. And then we can remove the AllocateAcpiNvsMemoryBelow4G() function altogether. > // > - // Allocate GDT and IDT in ACPI NVS and copy current GDT and IDT contents > + // Allocate GDT and IDT and copy current GDT and IDT contents > // > GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1; > IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1; > - Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize); > + Gdt = AllocateBootServiceMemory (GdtSize + IdtSize); > ASSERT (Gdt != NULL); > Idt = (VOID *)((UINTN)Gdt + GdtSize); > CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize); > @@ -243,7 +272,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 = (CPU_REGISTER_TABLE *)AllocateBootServiceMemory (TableSize); > ASSERT (RegisterTable != NULL); > > for (Index = 0; Index < NumberOfCpus; Index++) { > Thanks, Laszlo