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.126; helo=mga18.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (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 286CB211B76A9 for ; Mon, 14 Jan 2019 02:29:32 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Jan 2019 02:29:31 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,477,1539673200"; d="scan'208";a="311625169" Received: from shzintpr03.sh.intel.com (HELO [10.253.24.32]) ([10.239.4.100]) by fmsmga005.fm.intel.com with ESMTP; 14 Jan 2019 02:29:30 -0800 To: Laszlo Ersek , edk2-devel@lists.01.org Cc: Hao Wu , star.zeng@intel.com References: <1547393875-37188-1-git-send-email-star.zeng@intel.com> <1547393875-37188-2-git-send-email-star.zeng@intel.com> <279a5141-e843-2ee9-94bd-239c17a13975@redhat.com> From: "Zeng, Star" Message-ID: Date: Mon, 14 Jan 2019 18:29:00 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <279a5141-e843-2ee9-94bd-239c17a13975@redhat.com> Subject: Re: [PATCH 01/12] MdeModulePkg Variable: Add some missing changes for 9b18845 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Jan 2019 10:29:33 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Laszlo, On 2019/1/14 18:15, Laszlo Ersek wrote: > Hi Star, > > On 01/13/19 16:37, Star Zeng wrote: >> 9b18845a4b4cd1d2cf004cbc1cadf8a93ccb37ea changed the code >> which read from physical MMIO address to read from memory cache. >> >> The patch adds some missing changes for it. >> >> I found them when updating code for >> https://bugzilla.tianocore.org/show_bug.cgi?id=1323 >> Merge EmuVariable and Real variable driver. >> >> Cc: Jian J Wang >> Cc: Hao Wu >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Star Zeng >> --- >> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 12 +++++------- >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 6 +++--- >> 2 files changed, 8 insertions(+), 10 deletions(-) > > Please update the commit message to clarify the change. > > Commit 9b18845a4b4c ("MdeModulePkg Variable: Enhance variable > performance by reading from existed memory cache.", 2015-10-23) was a > performance optimization. Therefore, there can be two issues with it: > > (a) it may have missed another opportunity for performance improvement, > > (b) it may have introduced a regression. > > Can you please clarify the issue? The explanation should be part of the > commit message. > > Dependent on (a) vs. (b), the impact of this change could differ greatly. Oh, got your point. :) (a) is the issue, I can add it to be part of the commit message. If V2 patch series is needed, it will be included in that series. Thanks, Star > > Thanks! > Laszlo > > >> >> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >> index 443cf07144a1..99d487adac9e 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >> @@ -16,7 +16,7 @@ >> VariableServiceSetVariable() should also check authenticate data to avoid buffer overflow, >> integer overflow. It should also check attribute to avoid authentication bypass. >> >> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
>> (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP
>> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> @@ -262,13 +262,12 @@ UpdateVariableStore ( >> UINT8 *CurrBuffer; >> EFI_LBA LbaNumber; >> UINTN Size; >> - EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader; >> VARIABLE_STORE_HEADER *VolatileBase; >> EFI_PHYSICAL_ADDRESS FvVolHdr; >> EFI_PHYSICAL_ADDRESS DataPtr; >> EFI_STATUS Status; >> >> - FwVolHeader = NULL; >> + FvVolHdr = 0; >> DataPtr = DataPtrIndex; >> >> // >> @@ -281,7 +280,6 @@ UpdateVariableStore ( >> Status = Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); >> ASSERT_EFI_ERROR (Status); >> >> - FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *) ((UINTN) FvVolHdr); >> // >> // Data Pointer should point to the actual Address where data is to be >> // written. >> @@ -290,7 +288,7 @@ UpdateVariableStore ( >> DataPtr += mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase; >> } >> >> - if ((DataPtr + DataSize) > ((EFI_PHYSICAL_ADDRESS) (UINTN) ((UINT8 *) FwVolHeader + FwVolHeader->FvLength))) { >> + if ((DataPtr + DataSize) > (FvVolHdr + mNvFvHeaderCache->FvLength)) { >> return EFI_OUT_OF_RESOURCES; >> } >> } else { >> @@ -317,7 +315,7 @@ UpdateVariableStore ( >> // >> // If we are here we are dealing with Non-Volatile Variables. >> // >> - LinearOffset = (UINTN) FwVolHeader; >> + LinearOffset = (UINTN) FvVolHdr; >> CurrWritePtr = (UINTN) DataPtr; >> CurrWriteSize = DataSize; >> CurrBuffer = Buffer; >> @@ -2739,7 +2737,7 @@ UpdateVariable ( >> } >> } >> >> - State = Variable->CurrPtr->State; >> + State = CacheVariable->CurrPtr->State; >> State &= VAR_DELETED; >> >> Status = UpdateVariableStore ( >> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> index 23186176be75..f7185df3a7eb 100644 >> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c >> @@ -3,7 +3,7 @@ >> and volatile storage space and install variable architecture protocol. >> >> Copyright (C) 2013, Red Hat, Inc. >> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
>> (C) Copyright 2015 Hewlett Packard Enterprise Development LP
>> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD License >> @@ -402,8 +402,8 @@ FtwNotificationEvent ( >> // >> // Mark the variable storage region of the FLASH as RUNTIME. >> // >> - VariableStoreBase = NvStorageVariableBase + (((EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)(NvStorageVariableBase))->HeaderLength); >> - VariableStoreLength = ((VARIABLE_STORE_HEADER *)(UINTN)VariableStoreBase)->Size; >> + VariableStoreBase = NvStorageVariableBase + mNvFvHeaderCache->HeaderLength; >> + VariableStoreLength = mNvVariableCache->Size; >> BaseAddress = VariableStoreBase & (~EFI_PAGE_MASK); >> Length = VariableStoreLength + (VariableStoreBase - BaseAddress); >> Length = (Length + EFI_PAGE_SIZE - 1) & (~EFI_PAGE_MASK); >>