Re: [Patch] BaseTools: PI 1.6 to support FV extended header contain FV used size

["Gao, Liming" <liming.gao@intel.com>]

I have two comments here.

1) GenFv: ExtHeader UsedSize can directly be updated. You don't need to allocate new buffer for it. Besides, VtfFileFlag is not required to be listed in GenFvInternalLib.h. 
   
2) Genfds: self.ExtEntrySize), self.ExtEntryType and self.UsedSize are hard value. You can directly use value instead of the local variable.

>-----Original Message-----
>From: Zhu, Yonghong
>Sent: Thursday, September 28, 2017 4:22 PM
>To: edk2-devel@lists.01.org
>Cc: Feng, YunhuaX <yunhuax.feng@intel.com>; Gao, Liming
><liming.gao@intel.com>
>Subject: [Patch] BaseTools: PI 1.6 to support FV extended header contain FV
>used size
>
>From: Yunhua Feng <yunhuax.feng@intel.com>
>
>Per PI 1.6 we added an FV Extended Header entry that would contain the
>size of the FV that was in use.
>
>Cc: Liming Gao <liming.gao@intel.com>
>Cc: Yonghong Zhu <yonghong.zhu@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yunhua Feng <yunhuax.feng@intel.com>
>---
> BaseTools/Source/C/GenFv/GenFvInternalLib.c          | 19
>+++++++++++++++++--
> BaseTools/Source/C/GenFv/GenFvInternalLib.h          |  3 ++-
> BaseTools/Source/C/Include/Common/PiFirmwareVolume.h |  6 ++++++
> BaseTools/Source/Python/GenFds/FdfParser.py          |  2 +-
> BaseTools/Source/Python/GenFds/Fv.py                 | 20
>+++++++++++++++++++-
> 5 files changed, 45 insertions(+), 5 deletions(-)
>
>diff --git a/BaseTools/Source/C/GenFv/GenFvInternalLib.c
>b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
>index 01c862e..5b219b3 100644
>--- a/BaseTools/Source/C/GenFv/GenFvInternalLib.c
>+++ b/BaseTools/Source/C/GenFv/GenFvInternalLib.c
>@@ -42,10 +42,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>KIND, EITHER EXPRESS OR IMPLIED.
> #define ARMT_UNCONDITIONAL_JUMP_INSTRUCTION       0xEB000000
> #define ARM64_UNCONDITIONAL_JUMP_INSTRUCTION      0x14000000
>
> BOOLEAN mArm = FALSE;
> STATIC UINT32   MaxFfsAlignment = 0;
>+BOOLEAN VtfFileFlag = FALSE;
>
> EFI_GUID  mEfiFirmwareVolumeTopFileGuid       =
>EFI_FFS_VOLUME_TOP_FILE_GUID;
> EFI_GUID  mFileGuidArray [MAX_NUMBER_OF_FILES_IN_FV];
> EFI_GUID  mZeroGuid                           = {0x0, 0x0, 0x0, {0x0, 0x0, 0x0, 0x0, 0x0,
>0x0, 0x0, 0x0}};
> EFI_GUID  mDefaultCapsuleGuid                 = {0x3B6686BD, 0x0D76, 0x4030,
>{ 0xB7, 0x0E, 0xB5, 0x51, 0x9E, 0x2F, 0xC5, 0xA0 }};
>@@ -598,10 +599,11 @@ Returns:
> {
>   EFI_FFS_FILE_HEADER *PadFile;
>   UINTN               PadFileSize;
>   UINT32              NextFfsHeaderSize;
>   UINT32              CurFfsHeaderSize;
>+  EFI_FIRMWARE_VOLUME_EXT_ENTRY_USED_SIZE_TYPE *FvUsedSize;
>
>   CurFfsHeaderSize = sizeof (EFI_FFS_FILE_HEADER);
>   //
>   // Verify input parameters.
>   //
>@@ -703,10 +705,25 @@ Returns:
>
>   if (ExtHeader != NULL) {
>     //
>     // Copy Fv Extension Header and Set Fv Extension header offset
>     //
>+    if (ExtHeader->ExtHeaderSize > sizeof
>(EFI_FIRMWARE_VOLUME_EXT_HEADER)) {
>+      FvUsedSize =
>malloc(sizeof(EFI_FIRMWARE_VOLUME_EXT_ENTRY_USED_SIZE_TYPE));
>+      if (FvUsedSize == NULL) {
>+        return EFI_OUT_OF_RESOURCES;
>+      }
>+      memcpy(FvUsedSize, ExtHeader + 1,
>sizeof(EFI_FIRMWARE_VOLUME_EXT_ENTRY_USED_SIZE_TYPE));
>+      if (FvUsedSize->Hdr.ExtEntryType == EFI_FV_EXT_TYPE_USED_SIZE_TYPE)
>{
>+        if (VtfFileFlag) {
>+          FvUsedSize->UsedSize = mFvTotalSize;
>+        } else {
>+          FvUsedSize->UsedSize = mFvTakenSize;
>+        }
>+        memcpy(ExtHeader + 1, FvUsedSize,
>sizeof(EFI_FIRMWARE_VOLUME_EXT_ENTRY_USED_SIZE_TYPE));
>+      }
>+     }
>     memcpy ((UINT8 *)PadFile + CurFfsHeaderSize, ExtHeader, ExtHeader-
>>ExtHeaderSize);
>     ((EFI_FIRMWARE_VOLUME_HEADER *) FvImage->FileImage)-
>>ExtHeaderOffset = (UINT16) ((UINTN) ((UINT8 *)PadFile + CurFfsHeaderSize)
>- (UINTN) FvImage->FileImage);
> 	  //
> 	  // Make next file start at QWord Boundry
> 	  //
>@@ -3057,16 +3074,14 @@ Returns:
>   UINTN               FfsFileSize;
>   UINTN               FvExtendHeaderSize;
>   UINT32              FfsAlignment;
>   UINT32              FfsHeaderSize;
>   EFI_FFS_FILE_HEADER FfsHeader;
>-  BOOLEAN             VtfFileFlag;
>   UINTN               VtfFileSize;
>
>   FvExtendHeaderSize = 0;
>   VtfFileSize = 0;
>-  VtfFileFlag = FALSE;
>   fpin  = NULL;
>   Index = 0;
>
>   //
>   // Compute size for easy access later
>diff --git a/BaseTools/Source/C/GenFv/GenFvInternalLib.h
>b/BaseTools/Source/C/GenFv/GenFvInternalLib.h
>index f039fa4..9b1abe7 100644
>--- a/BaseTools/Source/C/GenFv/GenFvInternalLib.h
>+++ b/BaseTools/Source/C/GenFv/GenFvInternalLib.h
>@@ -1,10 +1,10 @@
> /** @file
> This file contains describes the public interfaces to the GenFvImage Library.
> The basic purpose of the library is to create Firmware Volume images.
>
>-Copyright (c) 2004 - 2014, Intel Corporation. All rights reserved.<BR>
>+Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
>License
> which accompanies this distribution.  The full text of the license may be found
>at
> http://opensource.org/licenses/bsd-license.php
>
>@@ -256,10 +256,11 @@ extern FV_INFO    mFvDataInfo;
> extern CAP_INFO   mCapDataInfo;
> extern EFI_GUID   mEfiFirmwareFileSystem2Guid;
> extern EFI_GUID   mEfiFirmwareFileSystem3Guid;
> extern UINT32     mFvTotalSize;
> extern UINT32     mFvTakenSize;
>+extern BOOLEAN    VtfFileFlag;
>
> extern EFI_PHYSICAL_ADDRESS mFvBaseAddress[];
> extern UINT32               mFvBaseAddressNumber;
> //
> // Local function prototypes
>diff --git a/BaseTools/Source/C/Include/Common/PiFirmwareVolume.h
>b/BaseTools/Source/C/Include/Common/PiFirmwareVolume.h
>index b5c2b03..c3089e8 100644
>--- a/BaseTools/Source/C/Include/Common/PiFirmwareVolume.h
>+++ b/BaseTools/Source/C/Include/Common/PiFirmwareVolume.h
>@@ -152,6 +152,12 @@ typedef struct {
>   //
>   // UINT8                             Data[1];
>   //
> } EFI_FIRMWARE_VOLUME_EXT_ENTRY_GUID_TYPE;
>
>+#define EFI_FV_EXT_TYPE_USED_SIZE_TYPE 0x03
>+typedef struct {
>+  EFI_FIRMWARE_VOLUME_EXT_ENTRY Hdr;
>+  UINT32 UsedSize;
>+} EFI_FIRMWARE_VOLUME_EXT_ENTRY_USED_SIZE_TYPE;
>+
> #endif
>diff --git a/BaseTools/Source/Python/GenFds/FdfParser.py
>b/BaseTools/Source/Python/GenFds/FdfParser.py
>index 499d0a6..b95afc7 100644
>--- a/BaseTools/Source/Python/GenFds/FdfParser.py
>+++ b/BaseTools/Source/Python/GenFds/FdfParser.py
>@@ -2309,11 +2309,11 @@ class FdfParser:
>             if name not in ("ERASE_POLARITY", "MEMORY_MAPPED", \
>                            "STICKY_WRITE", "LOCK_CAP", "LOCK_STATUS",
>"WRITE_ENABLED_CAP", \
>                            "WRITE_DISABLED_CAP", "WRITE_STATUS",
>"READ_ENABLED_CAP", \
>                            "READ_DISABLED_CAP", "READ_STATUS", "READ_LOCK_CAP", \
>                            "READ_LOCK_STATUS", "WRITE_LOCK_CAP",
>"WRITE_LOCK_STATUS", \
>-                           "WRITE_POLICY_RELIABLE", "WEAK_ALIGNMENT"):
>+                           "WRITE_POLICY_RELIABLE", "WEAK_ALIGNMENT",
>"FvUsedSizeEnable"):
>                 self.__UndoToken()
>                 return False
>
>             if not self.__IsToken( "="):
>                 raise Warning("expected '='", self.FileName, self.CurrentLineNumber)
>diff --git a/BaseTools/Source/Python/GenFds/Fv.py
>b/BaseTools/Source/Python/GenFds/Fv.py
>index 4b03adc..9d4196c 100644
>--- a/BaseTools/Source/Python/GenFds/Fv.py
>+++ b/BaseTools/Source/Python/GenFds/Fv.py
>@@ -49,10 +49,14 @@ class FV (FvClassObject):
>         self.FvAddressFileName = None
>         self.CapsuleName = None
>         self.FvBaseAddress = None
>         self.FvForceRebase = None
>         self.FvRegionInFD = None
>+        self.ExtEntryType = 0x3
>+        self.ExtEntrySize = 8
>+        self.UsedSize = 0
>+        self.UsedSizeEnable = False
>
>     ## AddToBuffer()
>     #
>     #   Generate Fv and add it to the Buffer
>     #
>@@ -305,10 +309,14 @@ class FV (FvClassObject):
>         self.FvInfFile.writelines("EFI_ERASE_POLARITY   = "       + \
>                                           ' %s' %ErasePloarity    + \
>                                           T_CHAR_LF)
>         if not (self.FvAttributeDict == None):
>             for FvAttribute in self.FvAttributeDict.keys() :
>+                if FvAttribute == "FvUsedSizeEnable":
>+                    if self.FvAttributeDict[FvAttribute].upper() in ('TRUE', '1') :
>+                        self.UsedSizeEnable = True
>+                    continue
>                 self.FvInfFile.writelines("EFI_"            + \
>                                           FvAttribute       + \
>                                           ' = '             + \
>                                           self.FvAttributeDict[FvAttribute] + \
>                                           T_CHAR_LF )
>@@ -320,16 +328,26 @@ class FV (FvClassObject):
>
>         #
>         # Generate FV extension header file
>         #
>         if self.FvNameGuid == None or self.FvNameGuid == '':
>-            if len(self.FvExtEntryType) > 0:
>+            if len(self.FvExtEntryType) > 0 or self.UsedSizeEnable:
>                 GenFdsGlobalVariable.ErrorLogger("FV Extension Header Entries
>declared for %s with no FvNameGuid declaration." % (self.UiFvName))
>
>         if self.FvNameGuid <> None and self.FvNameGuid <> '':
>             TotalSize = 16 + 4
>             Buffer = ''
>+            if self.UsedSizeEnable:
>+                TotalSize += (4 + 4)
>+                ## define EFI_FV_EXT_TYPE_USED_SIZE_TYPE 0x03
>+                #typedef  struct
>+                # {
>+                #    EFI_FIRMWARE_VOLUME_EXT_ENTRY Hdr;
>+                #    UINT32 UsedSize;
>+                # } EFI_FIRMWARE_VOLUME_EXT_ENTRY_USED_SIZE_TYPE;
>+                Buffer += pack('HHL', (self.ExtEntrySize), (self.ExtEntryType),
>(self.UsedSize))
>+
>             if self.FvNameString == 'TRUE':
>                 #
>                 # Create EXT entry for FV UI name
>                 # This GUID is used: A67DF1FA-8DE8-4E98-AF09-4BDF2EFFBC7C
>                 #
>--
>2.6.1.windows.1