Re: [edk2] [PATCH V2 01/15] MdeModulePkg Variable: Add some missing changes for 9b18845
On 01/14/19 16:19, Star Zeng wrote: > To improve performance 9b18845a4b4cd1d2cf004cbc1cadf8a93ccb37ea > changed the code which read from physical MMIO address to read > from memory cache, but it missed some places that could be updated > at the same away for performance optimization. This commit message looks good to me; I have one suggestion only: please consider replacing "at the same away" with just "the same way", when you push the patch. (I haven't checked anything else in this patch, so I think I shouldn't respond with an A-b.) Thank you! Laszlo > > The patch updates these places as supplementary. > > 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(-) > > 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_ADDRESSFvVolHdr; >EFI_PHYSICAL_ADDRESSDataPtr; >EFI_STATUS Status; > > - FwVolHeader = NULL; > + FvVolHdr= 0; >DataPtr = DataPtrIndex; > >// > @@ -281,7 +280,6 @@ UpdateVariableStore ( > Status = Fvb->GetPhysicalAddress(Fvb, ); > 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 &
Re: [edk2] [PATCH V2 01/15] MdeModulePkg Variable: Add some missing changes for 9b18845
Reviewed-by: Hao Wu Best Regards, Hao Wu > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Star Zeng > Sent: Monday, January 14, 2019 11:20 PM > To: edk2-devel@lists.01.org > Cc: Wu, Hao A; Zeng, Star > Subject: [edk2] [PATCH V2 01/15] MdeModulePkg Variable: Add some > missing changes for 9b18845 > > To improve performance 9b18845a4b4cd1d2cf004cbc1cadf8a93ccb37ea > changed the code which read from physical MMIO address to read > from memory cache, but it missed some places that could be updated > at the same away for performance optimization. > > The patch updates these places as supplementary. > > 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(-) > > 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_ADDRESSFvVolHdr; >EFI_PHYSICAL_ADDRESSDataPtr; >EFI_STATUS Status; > > - FwVolHeader = NULL; > + FvVolHdr= 0; >DataPtr = DataPtrIndex; > >// > @@ -281,7 +280,6 @@ UpdateVariableStore ( > Status = Fvb->GetPhysicalAddress(Fvb, ); > 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 =