hi Michael, Thanks for your review. On Tue, 20 Apr 2021 at 21:26, Kinney, Michael D wrote: > Hi, > > I think this patch is functional, but there are a few things that can be > improved. > > 1) We should not use of the hard coded constants for the > ImageCapsuleSupport values. > The UEFI spec has #defines for these values, and we need to figure out > how to add > those define values in the scope of the FMP Capsule Image Header. > Perhaps add the > values to FmpCapsuleImageHeaderClass in FmpCapsuleHeader.py: > > class FmpCapsuleImageHeaderClass (object): > > CAPSULE_SUPPORT_AUTHENTICATION = 0x0000000000000001 > CAPSULE_SUPPORT_DEPENDENCY = 0x0000000000000002 > > From GenerateCapsule.py, you can use these values to set > ImageCapsuleSupport. > > Value 0 is not defined by the UEFI Spec. Though that appears to be the > value used > for a raw payload without authentication or dependency information. > > Since GenerateCapsule does not import FmpCapsuleImageHeaderClass, these > defines > may have to be added to the FmpCapsuleHeaderClass instead. > Okay. I will make the changes as per your suggestion. > > 2) The ImageCapsuleSupport parameter is added in the middle of the python > function arguments. > Since this is a new field added at the end of the V3 structure, I > recommend we add arguments > at the end of the argument list. This will be an optional argument, so > any existing use > of these methods for V2 use cases will not be impacted. These python > methods can be > potentially used by other consumers, so we should always consider > backwards compatibility > when updating BaseTools/Source/Python/Common areas. > I believe that adding support for compatibility with previous versions will be a more involved effort. If you look at the code, the way it is right now, the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER structure currently supports version 2 -- there is no provision for generating a capsule with version 1 for example. So the way the code is structured right now, we can only support one version. So, if you are looking for having support for previous versions as well, I think that would require adding support for passing the structure version that we need, and then based on. this information, the structure would have certain fields present/absent. Would it be fine to add this field in the structure for now. If you think it necessary, support for multiple versions of the structure can be added as a separate exercise. Is my understanding of your comment correct, or am I missing something. -sughosh > > Thanks, > > Mike > > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of Sughosh > Ganu > > Sent: Monday, April 19, 2021 4:40 AM > > To: devel@edk2.groups.io > > Cc: Feng, Bob C ; Liming Gao < > gaoliming@byosoft.com.cn>; Chen, Christine ; > > Sughosh Ganu > > Subject: [edk2-devel] [PATCH] BaseTools: GenerateCapsule.py: Add support > for version 3 of FMP Image Header structure > > > > Add support for the ImageCapsuleSupport field, introduced in version 3 > > of the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER structure. This > > structure member is used to indicate if the corresponding payload has > > support for authentication and dependency. > > > > Signed-off-by: Sughosh Ganu > > --- > > .../Source/Python/Capsule/GenerateCapsule.py | 5 +++- > > .../Common/Uefi/Capsule/FmpCapsuleHeader.py | 26 +++++++++++++------ > > 2 files changed, 22 insertions(+), 9 deletions(-) > > > > diff --git a/BaseTools/Source/Python/Capsule/GenerateCapsule.py > b/BaseTools/Source/Python/Capsule/GenerateCapsule.py > > index a8de988253..6419a2ac6f 100644 > > --- a/BaseTools/Source/Python/Capsule/GenerateCapsule.py > > +++ b/BaseTools/Source/Python/Capsule/GenerateCapsule.py > > @@ -561,6 +561,7 @@ if __name__ == '__main__': > > print ('GenerateCapsule: error:' + str(Msg)) > > sys.exit (1) > > for SinglePayloadDescriptor in PayloadDescriptorList: > > + ImageCapsuleSupport = 0x0000000000000000 > > Result = SinglePayloadDescriptor.Payload > > try: > > FmpPayloadHeader.FwVersion = > SinglePayloadDescriptor.FwVersion > > @@ -575,6 +576,7 @@ if __name__ == '__main__': > > if SinglePayloadDescriptor.UseDependency: > > CapsuleDependency.Payload = Result > > CapsuleDependency.DepexExp = > SinglePayloadDescriptor.DepexExp > > + ImageCapsuleSupport |= 0x0000000000000002 > > Result = CapsuleDependency.Encode () > > if args.Verbose: > > CapsuleDependency.DumpInfo () > > @@ -607,13 +609,14 @@ if __name__ == '__main__': > > FmpAuthHeader.MonotonicCount = > SinglePayloadDescriptor.MonotonicCount > > FmpAuthHeader.CertData = CertData > > FmpAuthHeader.Payload = Result > > + ImageCapsuleSupport |= 0x0000000000000001 > > Result = FmpAuthHeader.Encode () > > if args.Verbose: > > FmpAuthHeader.DumpInfo () > > except: > > print ('GenerateCapsule: error: can not encode FMP > Auth Header') > > sys.exit (1) > > - FmpCapsuleHeader.AddPayload (SinglePayloadDescriptor.Guid, > Result, HardwareInstance = > > SinglePayloadDescriptor.HardwareInstance, UpdateImageIndex = > SinglePayloadDescriptor.UpdateImageIndex) > > + FmpCapsuleHeader.AddPayload (SinglePayloadDescriptor.Guid, > Result, ImageCapsuleSupport, HardwareInstance = > > SinglePayloadDescriptor.HardwareInstance, UpdateImageIndex = > SinglePayloadDescriptor.UpdateImageIndex) > > try: > > for EmbeddedDriver in EmbeddedDriverDescriptorList: > > FmpCapsuleHeader.AddEmbeddedDriver(EmbeddedDriver) > > diff --git > a/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py > > b/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py > > index 91d24919c4..a2a5cf0db8 100644 > > --- a/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py > > +++ b/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py > > @@ -47,14 +47,19 @@ class FmpCapsuleImageHeaderClass (object): > > # /// therefore can be modified without changing the Auth data. > > # /// > > # UINT64 UpdateHardwareInstance; > > + # > > + # /// > > + # /// Bits which indicate authentication and depex information > for the image that follows this structure > > + # /// > > + # UINT64 ImageCapsuleSupport > > # } EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER; > > # > > - # #define > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION 0x00000002 > > + # #define > EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION 0x00000003 > > > > - _StructFormat = ' > + _StructFormat = ' > _StructSize = struct.calcsize (_StructFormat) > > > > - EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION = > 0x00000002 > > + EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION = > 0x00000003 > > > > def __init__ (self): > > self._Valid = False > > @@ -64,6 +69,7 @@ class FmpCapsuleImageHeaderClass (object): > > self.UpdateImageSize = 0 > > self.UpdateVendorCodeSize = 0 > > self.UpdateHardwareInstance = 0x0000000000000000 > > + self.ImageCapsuleSupport = 0x0000000000000000 > > self.Payload = b'' > > self.VendorCodeBytes = b'' > > > > @@ -78,7 +84,8 @@ class FmpCapsuleImageHeaderClass (object): > > 0,0,0, > > self.UpdateImageSize, > > self.UpdateVendorCodeSize, > > - self.UpdateHardwareInstance > > + self.UpdateHardwareInstance, > > + self.ImageCapsuleSupport > > ) > > self._Valid = True > > return FmpCapsuleImageHeader + self.Payload + > self.VendorCodeBytes > > @@ -86,7 +93,7 @@ class FmpCapsuleImageHeaderClass (object): > > def Decode (self, Buffer): > > if len (Buffer) < self._StructSize: > > raise ValueError > > - (Version, UpdateImageTypeId, UpdateImageIndex, r0, r1, r2, > UpdateImageSize, UpdateVendorCodeSize, > > UpdateHardwareInstance) = \ > > + (Version, UpdateImageTypeId, UpdateImageIndex, r0, r1, r2, > UpdateImageSize, UpdateVendorCodeSize, > > UpdateHardwareInstance, ImageCapsuleSupport) = \ > > struct.unpack ( > > self._StructFormat, > > Buffer[0:self._StructSize] > > @@ -105,6 +112,7 @@ class FmpCapsuleImageHeaderClass (object): > > self.UpdateImageSize = UpdateImageSize > > self.UpdateVendorCodeSize = UpdateVendorCodeSize > > self.UpdateHardwareInstance = UpdateHardwareInstance > > + self.ImageCapsuleSupport = ImageCapsuleSupport > > self.Payload = > Buffer[self._StructSize:self._StructSize + UpdateImageSize] > > self.VendorCodeBytes = Buffer[self._StructSize + > UpdateImageSize:] > > self._Valid = True > > @@ -119,6 +127,7 @@ class FmpCapsuleImageHeaderClass (object): > > print > ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateImageSize = > {UpdateImageSize:08X}'.format > > (UpdateImageSize = self.UpdateImageSize)) > > print > ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateVendorCodeSize = > {UpdateVendorCodeSize:08X}'.format > > (UpdateVendorCodeSize = self.UpdateVendorCodeSize)) > > print > ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateHardwareInstance = > > {UpdateHardwareInstance:016X}'.format (UpdateHardwareInstance = > self.UpdateHardwareInstance)) > > + print > ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.ImageCapsuleSupport = > {ImageCapsuleSupport:016X}'.format > > (ImageCapsuleSupport = self.ImageCapsuleSupport)) > > print ('sizeof (Payload) > = {Size:08X}'.format (Size = len > > (self.Payload))) > > print ('sizeof (VendorCodeBytes) > = {Size:08X}'.format (Size = len > > (self.VendorCodeBytes))) > > > > @@ -172,8 +181,8 @@ class FmpCapsuleHeaderClass (object): > > raise ValueError > > return self._EmbeddedDriverList[Index] > > > > - def AddPayload (self, UpdateImageTypeId, Payload = b'', > VendorCodeBytes = b'', HardwareInstance = 0, UpdateImageIndex > > = 1): > > - self._PayloadList.append ((UpdateImageTypeId, Payload, > VendorCodeBytes, HardwareInstance, UpdateImageIndex)) > > + def AddPayload (self, UpdateImageTypeId, Payload = b'', > ImageCapsuleSupport = 0, VendorCodeBytes = b'', > > HardwareInstance = 0, UpdateImageIndex = 1): > > + self._PayloadList.append ((UpdateImageTypeId, Payload, > ImageCapsuleSupport, VendorCodeBytes, HardwareInstance, > > UpdateImageIndex)) > > > > def GetFmpCapsuleImageHeader (self, Index): > > if Index >= len (self._FmpCapsuleImageHeaderList): > > @@ -198,13 +207,14 @@ class FmpCapsuleHeaderClass (object): > > self._ItemOffsetList.append (Offset) > > Offset = Offset + len (EmbeddedDriver) > > Index = 1 > > - for (UpdateImageTypeId, Payload, VendorCodeBytes, > HardwareInstance, UpdateImageIndex) in self._PayloadList: > > + for (UpdateImageTypeId, Payload, ImageCapsuleSupport, > VendorCodeBytes, HardwareInstance, UpdateImageIndex) in > > self._PayloadList: > > FmpCapsuleImageHeader = FmpCapsuleImageHeaderClass () > > FmpCapsuleImageHeader.UpdateImageTypeId = > UpdateImageTypeId > > FmpCapsuleImageHeader.UpdateImageIndex = > UpdateImageIndex > > FmpCapsuleImageHeader.Payload = Payload > > FmpCapsuleImageHeader.VendorCodeBytes = > VendorCodeBytes > > FmpCapsuleImageHeader.UpdateHardwareInstance = > HardwareInstance > > + FmpCapsuleImageHeader.ImageCapsuleSupport = > ImageCapsuleSupport > > FmpCapsuleImage = FmpCapsuleImageHeader.Encode () > > FmpCapsuleData = FmpCapsuleData + FmpCapsuleImage > > > > -- > > 2.17.1 > > > > > > > > > > > >