From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 6765921E48F50 for ; Wed, 24 Jan 2018 07:44:23 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7995E4A6FF; Wed, 24 Jan 2018 15:49:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-13.rdu2.redhat.com [10.10.121.13]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0254A60C95; Wed, 24 Jan 2018 15:49:49 +0000 (UTC) From: Laszlo Ersek To: Dandan Bi , edk2-devel@lists.01.org Cc: Eric Dong , Liming Gao References: <1516780916-6364-1-git-send-email-dandan.bi@intel.com> <1516780916-6364-2-git-send-email-dandan.bi@intel.com> <00ecccd6-6e43-9433-7db1-244adc00f0d7@redhat.com> Message-ID: Date: Wed, 24 Jan 2018 16:49:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <00ecccd6-6e43-9433-7db1-244adc00f0d7@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 24 Jan 2018 15:49:51 +0000 (UTC) Subject: Re: [patch 1/3] UefiCpuPkg/S3Resume: Remove useless pref code X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Jan 2018 15:44:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit small update to my review comments: On 01/24/18 16:48, Laszlo Ersek wrote: > On 01/24/18 09:01, Dandan Bi wrote: >> Our new performance infrastructure can support to dump performance >> date form ACPI table in OS. So we can remove the old pref code to >> write performance data to OS. Again, please reference the TianoCore BZ and/or commit hashes and/or mailing list subjects/URLs, for specifying "new performance infrastructure". With that change, my R-b stands. Thanks Laszlo >> >> Cc: Eric Dong >> Cc: Laszlo Ersek >> Cc: Liming Gao >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Dandan Bi >> --- >> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 131 --------------------- >> .../Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 3 +- >> 2 files changed, 1 insertion(+), 133 deletions(-) >> >> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> index d7d2a4d..4d77689 100644 >> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> @@ -20,11 +20,10 @@ >> >> #include >> >> #include >> #include >> -#include >> #include >> #include >> #include >> #include >> #include >> @@ -284,136 +283,10 @@ GLOBAL_REMOVE_IF_UNREFERENCED IA32_GDT mGdtEntries[] = { >> GLOBAL_REMOVE_IF_UNREFERENCED CONST IA32_DESCRIPTOR mGdt = { >> sizeof (mGdtEntries) - 1, >> (UINTN) mGdtEntries >> }; >> >> -/** >> - Performance measure function to get S3 detailed performance data. >> - >> - This function will getS3 detailed performance data and saved in pre-reserved ACPI memory. >> -**/ >> -VOID >> -WriteToOsS3PerformanceData ( >> - VOID >> - ) >> -{ >> - EFI_STATUS Status; >> - EFI_PHYSICAL_ADDRESS mAcpiLowMemoryBase; >> - PERF_HEADER *PerfHeader; >> - PERF_DATA *PerfData; >> - UINT64 Ticker; >> - UINTN Index; >> - EFI_PEI_READ_ONLY_VARIABLE2_PPI *VariableServices; >> - UINTN VarSize; >> - UINTN LogEntryKey; >> - CONST VOID *Handle; >> - CONST CHAR8 *Token; >> - CONST CHAR8 *Module; >> - UINT64 StartTicker; >> - UINT64 EndTicker; >> - UINT64 StartValue; >> - UINT64 EndValue; >> - BOOLEAN CountUp; >> - UINT64 Freq; >> - >> - // >> - // Retrieve time stamp count as early as possible >> - // >> - Ticker = GetPerformanceCounter (); >> - >> - Freq = GetPerformanceCounterProperties (&StartValue, &EndValue); >> - >> - Freq = DivU64x32 (Freq, 1000); >> - >> - Status = PeiServicesLocatePpi ( >> - &gEfiPeiReadOnlyVariable2PpiGuid, >> - 0, >> - NULL, >> - (VOID **) &VariableServices >> - ); >> - if (EFI_ERROR (Status)) { >> - return; >> - } >> - >> - VarSize = sizeof (EFI_PHYSICAL_ADDRESS); >> - Status = VariableServices->GetVariable ( >> - VariableServices, >> - L"PerfDataMemAddr", >> - &gPerformanceProtocolGuid, >> - NULL, >> - &VarSize, >> - &mAcpiLowMemoryBase >> - ); >> - if (EFI_ERROR (Status)) { >> - DEBUG ((EFI_D_ERROR, "Fail to retrieve variable to log S3 performance data \n")); >> - return; >> - } >> - >> - PerfHeader = (PERF_HEADER *) (UINTN) mAcpiLowMemoryBase; >> - >> - if (PerfHeader->Signiture != PERFORMANCE_SIGNATURE) { >> - DEBUG ((EFI_D_ERROR, "Performance data in ACPI memory get corrupted! \n")); >> - return; >> - } >> - >> - // >> - // Record total S3 resume time. >> - // >> - if (EndValue >= StartValue) { >> - PerfHeader->S3Resume = Ticker - StartValue; >> - CountUp = TRUE; >> - } else { >> - PerfHeader->S3Resume = StartValue - Ticker; >> - CountUp = FALSE; >> - } >> - >> - // >> - // Get S3 detailed performance data >> - // >> - Index = 0; >> - LogEntryKey = 0; >> - while ((LogEntryKey = GetPerformanceMeasurement ( >> - LogEntryKey, >> - &Handle, >> - &Token, >> - &Module, >> - &StartTicker, >> - &EndTicker)) != 0) { >> - if (EndTicker != 0) { >> - PerfData = &PerfHeader->S3Entry[Index]; >> - >> - // >> - // Use File Handle to specify the different performance log for PEIM. >> - // File Handle is the base address of PEIM FFS file. >> - // >> - if ((AsciiStrnCmp (Token, "PEIM", PEI_PERFORMANCE_STRING_SIZE) == 0) && (Handle != NULL)) { >> - AsciiSPrint (PerfData->Token, PERF_TOKEN_LENGTH, "0x%11p", Handle); >> - } else { >> - AsciiStrnCpyS (PerfData->Token, PERF_TOKEN_SIZE, Token, PERF_TOKEN_LENGTH); >> - } >> - if (StartTicker == 1) { >> - StartTicker = StartValue; >> - } >> - if (EndTicker == 1) { >> - EndTicker = StartValue; >> - } >> - Ticker = CountUp? (EndTicker - StartTicker) : (StartTicker - EndTicker); >> - PerfData->Duration = (UINT32) DivU64x32 (Ticker, (UINT32) Freq); >> - >> - // >> - // Only Record > 1ms performance data so that more big performance can be recorded. >> - // >> - if ((Ticker > Freq) && (++Index >= PERF_PEI_ENTRY_MAX_NUM)) { >> - // >> - // Reach the maximum number of PEI performance log entries. >> - // >> - break; >> - } >> - } >> - } >> - PerfHeader->S3EntryNum = (UINT32) Index; >> -} >> >> /** >> The function will check if current waking vector is long mode. >> >> @param AcpiS3Context a pointer to a structure of ACPI_S3_CONTEXT >> @@ -602,14 +475,10 @@ S3ResumeBootOs ( >> // >> // report status code on S3 resume >> // >> REPORT_STATUS_CODE (EFI_PROGRESS_CODE, EFI_SOFTWARE_PEI_MODULE | EFI_SW_PEI_PC_OS_WAKE); >> >> - PERF_CODE ( >> - WriteToOsS3PerformanceData (); >> - ); >> - >> AsmTransferControl = (ASM_TRANSFER_CONTROL)(UINTN)PeiS3ResumeState->AsmTransferControl; >> if (Facs->XFirmwareWakingVector != 0) { >> // >> // Switch to native waking vector >> // >> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf >> index 15fa2d1..9522ede 100644 >> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf >> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf >> @@ -3,11 +3,11 @@ >> # >> # This module works with StandAloneBootScriptExecutor to S3 resume to OS. >> # This module will excute the boot script saved during last boot and after that, >> # control is passed to OS waking up handler. >> # >> -# Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.
>> +# Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.
>> # Copyright (c) 2017, AMD Incorporated. All rights reserved.
>> # >> # This program and the accompanying materials are >> # licensed and made available under the terms and conditions of the BSD License >> # which accompanies this distribution. The full text of the license may be found at >> @@ -71,11 +71,10 @@ >> PrintLib >> >> [Guids] >> gEfiBootScriptExecutorVariableGuid ## SOMETIMES_CONSUMES ## UNDEFINED # LockBox >> gEfiBootScriptExecutorContextGuid ## SOMETIMES_CONSUMES ## UNDEFINED # LockBox >> - gPerformanceProtocolGuid ## SOMETIMES_CONSUMES ## Variable:L"PerfDataMemAddr" >> ## SOMETIMES_CONSUMES ## HOB >> ## SOMETIMES_CONSUMES ## UNDEFINED # LockBox >> gEfiAcpiVariableGuid >> gEfiAcpiS3ContextGuid ## SOMETIMES_CONSUMES ## UNDEFINED # LockBox >> gEdkiiEndOfS3ResumeGuid ## SOMETIMES_CONSUMES ## UNDEFINED # Used to do smm communication >> > > Looks good to me. > > Specifically for use in OVMF, this logic has always been inactive, since > we don't yet have PEI-phase (r/o) variable access. > > Reviewed-by: Laszlo Ersek