Re: [PATCH 01/12] MdeModulePkg Variable: Add some missing changes for 9b18845

["Zeng, Star" <star.zeng@intel.com>]

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 <jian.j.wang@intel.com>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>>   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.<BR>
>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>>   (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
>>   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.<BR>
>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>>   (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
>>   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);
>>