Re: [edk2-devel] [PATCH] BaseTools: GenerateCapsule.py: Add support for version 3 of FMP Image Header structure

["Sughosh Ganu" <sughosh.ganu@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 13769 bytes --]

hi Michael,

On Wed, 21 Apr 2021 at 21:04, Kinney, Michael D <michael.d.kinney@intel.com>
wrote:

> Sughosh,
>
>
>
> My feedback on compatibility was for the parameter list to the Python
> methods.  Not the C structures.  The python modules in the Common
> directory could be used by other tools.
>

Okay, understood. I will make the changes and submit a v2 shortly. Thanks
for the explanation.

-sughosh


>
>
> Mike
>
>
>
> *From:* Sughosh Ganu <sughosh.ganu@linaro.org>
> *Sent:* Wednesday, April 21, 2021 12:02 AM
> *To:* Kinney, Michael D <michael.d.kinney@intel.com>
> *Cc:* devel@edk2.groups.io; Feng, Bob C <bob.c.feng@intel.com>; Liming
> Gao <gaoliming@byosoft.com.cn>; Chen, Christine <yuwei.chen@intel.com>
> *Subject:* Re: [edk2-devel] [PATCH] BaseTools: GenerateCapsule.py: Add
> support for version 3 of FMP Image Header structure
>
>
>
> hi Michael,
>
> Thanks for your review.
>
>
>
> On Tue, 20 Apr 2021 at 21:26, Kinney, Michael D <
> michael.d.kinney@intel.com> 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 <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 <bob.c.feng@intel.com>; Liming Gao <
> gaoliming@byosoft.com.cn>; Chen, Christine <yuwei.chen@intel.com>;
> > Sughosh Ganu <sughosh.ganu@linaro.org>
> > 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 <sughosh.ganu@linaro.org>
> > ---
> >  .../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 = '<I16sB3BIIQ'
> > +    _StructFormat = '<I16sB3BIIQQ'
> >      _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
> >
> >
> >
> > 
> >
>
>

[-- Attachment #2: Type: text/html, Size: 19908 bytes --]