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

Mon, 14 Jan 2019 02:29:47 -0800 

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.w...@intel.com>
Cc: Hao Wu <hao.a...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng <star.z...@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);



_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to