From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 978DA2222C23E for ; Fri, 26 Jan 2018 01:08:18 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jan 2018 01:13:48 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,415,1511856000"; d="scan'208";a="12766856" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.19]) ([10.239.9.19]) by orsmga007.jf.intel.com with ESMTP; 26 Jan 2018 01:13:47 -0800 To: Jian J Wang , edk2-devel@lists.01.org Cc: Laszlo Ersek , Jiewen Yao , Eric Dong References: <20180126090307.6872-1-jian.j.wang@intel.com> <20180126090307.6872-3-jian.j.wang@intel.com> From: "Ni, Ruiyu" Message-ID: <418df7fd-dc3c-aa0b-9348-37476cf8dadb@Intel.com> Date: Fri, 26 Jan 2018 17:13:46 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180126090307.6872-3-jian.j.wang@intel.com> Subject: Re: [PATCH 2/2] UefiCpuPkg/CpuDxe: remove all code to flush TLB for APs 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: Fri, 26 Jan 2018 09:08:19 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 1/26/2018 5:03 PM, Jian J Wang wrote: > The reason doing this is that we found that calling StartupAllAps() to > flush TLB for all APs in CpuDxe driver after changing page attributes > will spend a lot of time to complete. If there are many page attributes > update requests, the whole system performance will be slowed down > explicitly, including any shell command and UI operation. > > The solution is removing the flush operation for AP in CpuDxe driver > and let AP flush TLB after woken up. > > Cc: Ruiyu Ni > Cc: Jiewen Yao > Cc: Eric Dong > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > UefiCpuPkg/CpuDxe/CpuPageTable.c | 85 +++------------------------------------- > 1 file changed, 5 insertions(+), 80 deletions(-) > > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c > index a33ac5519e..a5bf0dfe28 100644 > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > @@ -89,70 +89,6 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { > > PAGE_TABLE_POOL *mPageTablePool = NULL; > > -/** > - Enable write protection function for AP. > - > - @param[in,out] Buffer The pointer to private data buffer. > -**/ > -VOID > -EFIAPI > -SyncCpuEnableWriteProtection ( > - IN OUT VOID *Buffer > - ) > -{ > - AsmWriteCr0 (AsmReadCr0 () | BIT16); > -} > - > -/** > - CpuFlushTlb function for AP. > - > - @param[in,out] Buffer The pointer to private data buffer. > -**/ > -VOID > -EFIAPI > -SyncCpuFlushTlb ( > - IN OUT VOID *Buffer > - ) > -{ > - CpuFlushTlb(); > -} > - > -/** > - Sync memory page attributes for AP. > - > - @param[in] Procedure A pointer to the function to be run on enabled APs of > - the system. > -**/ > -VOID > -SyncMemoryPageAttributesAp ( > - IN EFI_AP_PROCEDURE Procedure > - ) > -{ > - EFI_STATUS Status; > - EFI_MP_SERVICES_PROTOCOL *MpService; > - > - Status = gBS->LocateProtocol ( > - &gEfiMpServiceProtocolGuid, > - NULL, > - (VOID **)&MpService > - ); > - // > - // Synchronize the update with all APs > - // > - if (!EFI_ERROR (Status)) { > - Status = MpService->StartupAllAPs ( > - MpService, // This > - Procedure, // Procedure > - FALSE, // SingleThread > - NULL, // WaitEvent > - 0, // TimeoutInMicrosecsond > - NULL, // ProcedureArgument > - NULL // FailedCpuList > - ); > - ASSERT (Status == EFI_SUCCESS || Status == EFI_NOT_STARTED || Status == EFI_NOT_READY); > - } > -} > - > /** > Return current paging context. > > @@ -574,20 +510,6 @@ IsReadOnlyPageWriteProtected ( > return ((AsmReadCr0 () & BIT16) != 0); > } > > -/** > - Disable write protection function for AP. > - > - @param[in,out] Buffer The pointer to private data buffer. > -**/ > -VOID > -EFIAPI > -SyncCpuDisableWriteProtection ( > - IN OUT VOID *Buffer > - ) > -{ > - AsmWriteCr0 (AsmReadCr0() & ~BIT16); > -} > - > /** > Disable Write Protect on pages marked as read-only. > **/ > @@ -835,10 +757,13 @@ AssignMemoryPageAttributes ( > if (!EFI_ERROR(Status)) { > if ((PagingContext == NULL) && IsModified) { > // > - // Flush TLB as last step > + // Flush TLB as last step. > + // > + // Note: Don't flush TLB for APs here. It will take a lot of time to > + // complete, and then slow down boot performance of the whole system > + // if page attributes are requested frequently to update. > // Code change looks good. But comments look like we skip the sync due to performance. In fact, sync is unnecessary. How about comments like below (refine as you need): No need to flush TLB for APs here because: 1. when APs wake up from hlt, AP initialization code always sets CR3 2. when APs wake up from mwait/run loop, patch *UefiCpuPkg/MpInitLib: force flushing TLB for AP in mwait loop mode* sets CR3. With the comments refine, Reviewed-by: Ruiyu Ni > CpuFlushTlb(); > - SyncMemoryPageAttributesAp (SyncCpuFlushTlb); > } > } > > -- Thanks, Ray