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 1E55920972140 for ; Fri, 3 Aug 2018 09:46:00 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5D56140216F4; Fri, 3 Aug 2018 16:45:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-198.rdu2.redhat.com [10.10.120.198]) by smtp.corp.redhat.com (Postfix) with ESMTP id EA819215670D; Fri, 3 Aug 2018 16:45:57 +0000 (UTC) To: Eric Dong Cc: edk2-devel@lists.01.org, Ruiyu Ni , =?UTF-8?Q?Marvin_H=c3=a4user?= , Jeff Fan , Ard Biesheuvel References: <20180803021031.18244-1-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <8cf4b763-52c3-9d91-56e9-53e38bd71f12@redhat.com> Date: Fri, 3 Aug 2018 18:45:57 +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: <20180803021031.18244-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Fri, 03 Aug 2018 16:45:59 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Fri, 03 Aug 2018 16:45:59 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData function. 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, 03 Aug 2018 16:46:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Hello Eric, OVMF does not include CpuFeaturesPei / CpuFeaturesDxe, and so it doesn't consume this library. I can't provide test results, but I have some superficial comments. First, I'm adding Marvin and Jeff -- I *vaguely* recall that this issue may have been raised by Marvin. Hm... yes, it seems so: [edk2] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency. http://mid.mail-archive.com/VI1PR0801MB17908DB1F03A3C5F84545E1080690@VI1PR0801MB1790.eurprd08.prod.outlook.com https://lists.01.org/pipermail/edk2-devel/2018-May/025163.html I have three questions here: (1) Do we have a TianoCore BZ about this? (2) If not, does the currently proposed commit message capture the issue in enough detail? Should we reference Marvin's initial report and/or a TianoCore BZ (if we have one)? (3) The implementation seems to follow Jeff's idea. Marvin and Jeff, can you please help with the review? Either way, I propose the following two tags to be appended: Reported-by: Marvin Häuser Suggested-by: Fan Jeff And one technical question follows below: On 08/03/18 04:10, Eric Dong wrote: > Current code logic can't confirm CpuS3DataDxe driver start before > CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not > valid. Add implementation for AllocateAcpiCpuData function to remove > this assumption. > > Pass OS boot and resume from S3 test. > > Cc: Laszlo Ersek > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > .../DxeRegisterCpuFeaturesLib.c | 57 ++++++++++++++++++++-- > 1 file changed, 54 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > index 902a339529..0722db6c64 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c > @@ -207,11 +207,62 @@ AllocateAcpiCpuData ( > VOID > ) > { > + EFI_STATUS Status; > + UINTN NumberOfCpus; > + UINTN NumberOfEnabledProcessors; > + ACPI_CPU_DATA *AcpiCpuData; > + EFI_PHYSICAL_ADDRESS Address; > + UINTN TableSize; > + CPU_REGISTER_TABLE *RegisterTable; > + UINTN Index; > + EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer; > + > + Address = BASE_4GB - 1; > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)), > + &Address > + ); > + ASSERT_EFI_ERROR (Status); I understand that this code is for IA32 and X64 only, but still, Ard has recently introduced a DxeServicesLib API for this kind of allocation: it's called AllocatePeiAccessiblePages(). See commit a40e0b7aa918. (4) Is it feasible to use that function here (and in the second instance below)? >>From a library dependency standpoint, I think it should be fine: we are modifying the DXE instance of the RegisterCpuFeaturesLib class, so a dependency on DxeServicesLib should be in order. Now, if this allocation *must* be satisfied from below 4GB, even if the PEI phase has access to >=4GB RAM (as determined by PhitHob->EfiFreeMemoryTop > MAX_UINT32 ), then my suggestion is wrong (because in that case, AllocatePeiAccessiblePages() wouldn't restrict the allocation under 4GB). CC'ing Ard too. Thanks! Laszlo > + AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address; > + ASSERT (AcpiCpuData != NULL); > + > + GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors); > + AcpiCpuData->NumberOfCpus = (UINT32) NumberOfCpus; > + > // > - // CpuS3DataDxe will do it. > + // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs > // > - ASSERT (FALSE); > - return NULL; > + TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE); > + Address = BASE_4GB - 1; > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + EFI_SIZE_TO_PAGES (TableSize), > + &Address > + ); > + ASSERT_EFI_ERROR (Status); > + RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address; > + > + for (Index = 0; Index < NumberOfCpus; Index++) { > + Status = GetProcessorInformation (Index, &ProcessorInfoBuffer); > + ASSERT_EFI_ERROR (Status); > + > + RegisterTable[Index].InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId; > + RegisterTable[Index].TableLength = 0; > + RegisterTable[Index].AllocatedSize = 0; > + RegisterTable[Index].RegisterTableEntry = 0; > + > + RegisterTable[NumberOfCpus + Index].InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId; > + RegisterTable[NumberOfCpus + Index].TableLength = 0; > + RegisterTable[NumberOfCpus + Index].AllocatedSize = 0; > + RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0; > + } > + AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable; > + AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus); > + > + return AcpiCpuData; > } > > /** >