<div dir="ltr"><div>hi Michael,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 21 Apr 2021 at 21:04, Kinney, Michael D <<a href="mailto:michael.d.kinney@intel.com">michael.d.kinney@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div lang="EN-US" style="overflow-wrap: break-word;">
<div class="gmail-m_7938623777033397277WordSection1">
<p class="MsoNormal"><span class="gmail-m_7938623777033397277SpellE"><span>Sughosh</span></span><span>,<u></u><u></u></span></p>
<p class="MsoNormal"><span><u></u> <u></u></span></p>
<p class="MsoNormal"><span>My feedback on compatibility was for the parameter list to the Python methods.<span>
</span>Not the C structures.<span> </span>The python modules in the Common directory could be used by other tools.</span></p></div></div></blockquote><div><br></div><div>Okay, understood. I will make the changes and submit a v2 shortly. Thanks for the explanation.</div><div><br></div><div>-sughosh</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US" style="overflow-wrap: break-word;"><div class="gmail-m_7938623777033397277WordSection1"><p class="MsoNormal"><span><u></u><u></u></span></p>
<p class="MsoNormal"><span><u></u> <u></u></span></p>
<p class="MsoNormal"><span>Mike<u></u><u></u></span></p>
<p class="MsoNormal"><span><u></u> <u></u></span></p>
<div style="border-top:none;border-right:none;border-bottom:none;border-left:1.5pt solid blue;padding:0in 0in 0in 4pt">
<div>
<div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(225,225,225);padding:3pt 0in 0in">
<p class="MsoNormal"><b><span>From:</span></b><span> Sughosh Ganu <<a href="mailto:sughosh.ganu@linaro.org" target="_blank">sughosh.ganu@linaro.org</a>>
<br>
<b>Sent:</b> Wednesday, April 21, 2021 12:02 AM<br>
<b>To:</b> Kinney, Michael D <<a href="mailto:michael.d.kinney@intel.com" target="_blank">michael.d.kinney@intel.com</a>><br>
<b>Cc:</b> <a href="mailto:devel@edk2.groups.io" target="_blank">devel@edk2.groups.io</a>; Feng, Bob C <<a href="mailto:bob.c.feng@intel.com" target="_blank">bob.c.feng@intel.com</a>>; Liming Gao <<a href="mailto:gaoliming@byosoft.com.cn" target="_blank">gaoliming@byosoft.com.cn</a>>; Chen, Christine <<a href="mailto:yuwei.chen@intel.com" target="_blank">yuwei.chen@intel.com</a>><br>
<b>Subject:</b> Re: [edk2-devel] [PATCH] BaseTools: GenerateCapsule.py: Add support for version 3 of FMP Image Header structure<u></u><u></u></span></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">hi Michael,<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Thanks for your review.<u></u><u></u></p>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">On Tue, 20 Apr 2021 at 21:26, Kinney, Michael D <<a href="mailto:michael.d.kinney@intel.com" target="_blank">michael.d.kinney@intel.com</a>> wrote:<u></u><u></u></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">Hi,<br>
<br>
I think this patch is functional, but there are a few things that can be improved.<br>
<br>
1) We should not use of the hard coded constants for the ImageCapsuleSupport values.<br>
The UEFI spec has #defines for these values, and we need to figure out how to add<br>
those define values in the scope of the FMP Capsule Image Header. Perhaps add the<br>
values to FmpCapsuleImageHeaderClass in FmpCapsuleHeader.py:<br>
<br>
class FmpCapsuleImageHeaderClass (object):<br>
<br>
CAPSULE_SUPPORT_AUTHENTICATION = 0x0000000000000001<br>
CAPSULE_SUPPORT_DEPENDENCY = 0x0000000000000002<br>
<br>
From GenerateCapsule.py, you can use these values to set ImageCapsuleSupport.<br>
<br>
Value 0 is not defined by the UEFI Spec. Though that appears to be the value used<br>
for a raw payload without authentication or dependency information.<br>
<br>
Since GenerateCapsule does not import FmpCapsuleImageHeaderClass, these defines
<br>
may have to be added to the FmpCapsuleHeaderClass instead.<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Okay. I will make the changes as per your suggestion.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal"><br>
2) The ImageCapsuleSupport parameter is added in the middle of the python function arguments.<br>
Since this is a new field added at the end of the V3 structure, I recommend we add arguments<br>
at the end of the argument list. This will be an optional argument, so any existing use<br>
of these methods for V2 use cases will not be impacted. These python methods can be
<br>
potentially used by other consumers, so we should always consider backwards compatibility<br>
when updating BaseTools/Source/Python/Common areas.<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">-sughosh <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal" style="margin-bottom:12pt"><br>
Thanks,<br>
<br>
Mike<br>
<br>
> -----Original Message-----<br>
> From: <a href="mailto:devel@edk2.groups.io" target="_blank">devel@edk2.groups.io</a> <<a href="mailto:devel@edk2.groups.io" target="_blank">devel@edk2.groups.io</a>> On Behalf Of Sughosh Ganu<br>
> Sent: Monday, April 19, 2021 4:40 AM<br>
> To: <a href="mailto:devel@edk2.groups.io" target="_blank">devel@edk2.groups.io</a><br>
> Cc: Feng, Bob C <<a href="mailto:bob.c.feng@intel.com" target="_blank">bob.c.feng@intel.com</a>>; Liming Gao <<a href="mailto:gaoliming@byosoft.com.cn" target="_blank">gaoliming@byosoft.com.cn</a>>; Chen, Christine <<a href="mailto:yuwei.chen@intel.com" target="_blank">yuwei.chen@intel.com</a>>;<br>
> Sughosh Ganu <<a href="mailto:sughosh.ganu@linaro.org" target="_blank">sughosh.ganu@linaro.org</a>><br>
> Subject: [edk2-devel] [PATCH] BaseTools: GenerateCapsule.py: Add support for version 3 of FMP Image Header structure<br>
> <br>
> Add support for the ImageCapsuleSupport field, introduced in version 3<br>
> of the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER structure. This<br>
> structure member is used to indicate if the corresponding payload has<br>
> support for authentication and dependency.<br>
> <br>
> Signed-off-by: Sughosh Ganu <<a href="mailto:sughosh.ganu@linaro.org" target="_blank">sughosh.ganu@linaro.org</a>><br>
> ---<br>
> .../Source/Python/Capsule/GenerateCapsule.py | 5 +++-<br>
> .../Common/Uefi/Capsule/FmpCapsuleHeader.py | 26 +++++++++++++------<br>
> 2 files changed, 22 insertions(+), 9 deletions(-)<br>
> <br>
> diff --git a/BaseTools/Source/Python/Capsule/GenerateCapsule.py b/BaseTools/Source/Python/Capsule/GenerateCapsule.py<br>
> index a8de988253..6419a2ac6f 100644<br>
> --- a/BaseTools/Source/Python/Capsule/GenerateCapsule.py<br>
> +++ b/BaseTools/Source/Python/Capsule/GenerateCapsule.py<br>
> @@ -561,6 +561,7 @@ if __name__ == '__main__':<br>
> print ('GenerateCapsule: error:' + str(Msg))<br>
> sys.exit (1)<br>
> for SinglePayloadDescriptor in PayloadDescriptorList:<br>
> + ImageCapsuleSupport = 0x0000000000000000<br>
> Result = SinglePayloadDescriptor.Payload<br>
> try:<br>
> FmpPayloadHeader.FwVersion = SinglePayloadDescriptor.FwVersion<br>
> @@ -575,6 +576,7 @@ if __name__ == '__main__':<br>
> if SinglePayloadDescriptor.UseDependency:<br>
> CapsuleDependency.Payload = Result<br>
> CapsuleDependency.DepexExp = SinglePayloadDescriptor.DepexExp<br>
> + ImageCapsuleSupport |= 0x0000000000000002<br>
> Result = CapsuleDependency.Encode ()<br>
> if args.Verbose:<br>
> CapsuleDependency.DumpInfo ()<br>
> @@ -607,13 +609,14 @@ if __name__ == '__main__':<br>
> FmpAuthHeader.MonotonicCount = SinglePayloadDescriptor.MonotonicCount<br>
> FmpAuthHeader.CertData = CertData<br>
> FmpAuthHeader.Payload = Result<br>
> + ImageCapsuleSupport |= 0x0000000000000001<br>
> Result = FmpAuthHeader.Encode ()<br>
> if args.Verbose:<br>
> FmpAuthHeader.DumpInfo ()<br>
> except:<br>
> print ('GenerateCapsule: error: can not encode FMP Auth Header')<br>
> sys.exit (1)<br>
> - FmpCapsuleHeader.AddPayload (SinglePayloadDescriptor.Guid, Result, HardwareInstance =<br>
> SinglePayloadDescriptor.HardwareInstance, UpdateImageIndex = SinglePayloadDescriptor.UpdateImageIndex)<br>
> + FmpCapsuleHeader.AddPayload (SinglePayloadDescriptor.Guid, Result, ImageCapsuleSupport, HardwareInstance =<br>
> SinglePayloadDescriptor.HardwareInstance, UpdateImageIndex = SinglePayloadDescriptor.UpdateImageIndex)<br>
> try:<br>
> for EmbeddedDriver in EmbeddedDriverDescriptorList:<br>
> FmpCapsuleHeader.AddEmbeddedDriver(EmbeddedDriver)<br>
> diff --git a/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py<br>
> b/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py<br>
> index 91d24919c4..a2a5cf0db8 100644<br>
> --- a/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py<br>
> +++ b/BaseTools/Source/Python/Common/Uefi/Capsule/FmpCapsuleHeader.py<br>
> @@ -47,14 +47,19 @@ class FmpCapsuleImageHeaderClass (object):<br>
> # /// therefore can be modified without changing the Auth data.<br>
> # ///<br>
> # UINT64 UpdateHardwareInstance;<br>
> + #<br>
> + # ///<br>
> + # /// Bits which indicate authentication and depex information for the image that follows this structure<br>
> + # ///<br>
> + # UINT64 ImageCapsuleSupport<br>
> # } EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;<br>
> #<br>
> - # #define EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION 0x00000002<br>
> + # #define EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION 0x00000003<br>
> <br>
> - _StructFormat = '<I16sB3BIIQ'<br>
> + _StructFormat = '<I16sB3BIIQQ'<br>
> _StructSize = struct.calcsize (_StructFormat)<br>
> <br>
> - EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION = 0x00000002<br>
> + EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION = 0x00000003<br>
> <br>
> def __init__ (self):<br>
> self._Valid = False<br>
> @@ -64,6 +69,7 @@ class FmpCapsuleImageHeaderClass (object):<br>
> self.UpdateImageSize = 0<br>
> self.UpdateVendorCodeSize = 0<br>
> self.UpdateHardwareInstance = 0x0000000000000000<br>
> + self.ImageCapsuleSupport = 0x0000000000000000<br>
> self.Payload = b''<br>
> self.VendorCodeBytes = b''<br>
> <br>
> @@ -78,7 +84,8 @@ class FmpCapsuleImageHeaderClass (object):<br>
> 0,0,0,<br>
> self.UpdateImageSize,<br>
> self.UpdateVendorCodeSize,<br>
> - self.UpdateHardwareInstance<br>
> + self.UpdateHardwareInstance,<br>
> + self.ImageCapsuleSupport<br>
> )<br>
> self._Valid = True<br>
> return FmpCapsuleImageHeader + self.Payload + self.VendorCodeBytes<br>
> @@ -86,7 +93,7 @@ class FmpCapsuleImageHeaderClass (object):<br>
> def Decode (self, Buffer):<br>
> if len (Buffer) < self._StructSize:<br>
> raise ValueError<br>
> - (Version, UpdateImageTypeId, UpdateImageIndex, r0, r1, r2, UpdateImageSize, UpdateVendorCodeSize,<br>
> UpdateHardwareInstance) = \<br>
> + (Version, UpdateImageTypeId, UpdateImageIndex, r0, r1, r2, UpdateImageSize, UpdateVendorCodeSize,<br>
> UpdateHardwareInstance, ImageCapsuleSupport) = \<br>
> struct.unpack (<br>
> self._StructFormat,<br>
> Buffer[0:self._StructSize]<br>
> @@ -105,6 +112,7 @@ class FmpCapsuleImageHeaderClass (object):<br>
> self.UpdateImageSize = UpdateImageSize<br>
> self.UpdateVendorCodeSize = UpdateVendorCodeSize<br>
> self.UpdateHardwareInstance = UpdateHardwareInstance<br>
> + self.ImageCapsuleSupport = ImageCapsuleSupport<br>
> self.Payload = Buffer[self._StructSize:self._StructSize + UpdateImageSize]<br>
> self.VendorCodeBytes = Buffer[self._StructSize + UpdateImageSize:]<br>
> self._Valid = True<br>
> @@ -119,6 +127,7 @@ class FmpCapsuleImageHeaderClass (object):<br>
> print ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateImageSize = {UpdateImageSize:08X}'.format<br>
> (UpdateImageSize = self.UpdateImageSize))<br>
> print ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateVendorCodeSize = {UpdateVendorCodeSize:08X}'.format<br>
> (UpdateVendorCodeSize = self.UpdateVendorCodeSize))<br>
> print ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.UpdateHardwareInstance =<br>
> {UpdateHardwareInstance:016X}'.format (UpdateHardwareInstance = self.UpdateHardwareInstance))<br>
> + print ('EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER.ImageCapsuleSupport = {ImageCapsuleSupport:016X}'.format<br>
> (ImageCapsuleSupport = self.ImageCapsuleSupport))<br>
> print ('sizeof (Payload) = {Size:08X}'.format (Size = len<br>
> (self.Payload)))<br>
> print ('sizeof (VendorCodeBytes) = {Size:08X}'.format (Size = len<br>
> (self.VendorCodeBytes)))<br>
> <br>
> @@ -172,8 +181,8 @@ class FmpCapsuleHeaderClass (object):<br>
> raise ValueError<br>
> return self._EmbeddedDriverList[Index]<br>
> <br>
> - def AddPayload (self, UpdateImageTypeId, Payload = b'', VendorCodeBytes = b'', HardwareInstance = 0, UpdateImageIndex<br>
> = 1):<br>
> - self._PayloadList.append ((UpdateImageTypeId, Payload, VendorCodeBytes, HardwareInstance, UpdateImageIndex))<br>
> + def AddPayload (self, UpdateImageTypeId, Payload = b'', ImageCapsuleSupport = 0, VendorCodeBytes = b'',<br>
> HardwareInstance = 0, UpdateImageIndex = 1):<br>
> + self._PayloadList.append ((UpdateImageTypeId, Payload, ImageCapsuleSupport, VendorCodeBytes, HardwareInstance,<br>
> UpdateImageIndex))<br>
> <br>
> def GetFmpCapsuleImageHeader (self, Index):<br>
> if Index >= len (self._FmpCapsuleImageHeaderList):<br>
> @@ -198,13 +207,14 @@ class FmpCapsuleHeaderClass (object):<br>
> self._ItemOffsetList.append (Offset)<br>
> Offset = Offset + len (EmbeddedDriver)<br>
> Index = 1<br>
> - for (UpdateImageTypeId, Payload, VendorCodeBytes, HardwareInstance, UpdateImageIndex) in self._PayloadList:<br>
> + for (UpdateImageTypeId, Payload, ImageCapsuleSupport, VendorCodeBytes, HardwareInstance, UpdateImageIndex) in<br>
> self._PayloadList:<br>
> FmpCapsuleImageHeader = FmpCapsuleImageHeaderClass ()<br>
> FmpCapsuleImageHeader.UpdateImageTypeId = UpdateImageTypeId<br>
> FmpCapsuleImageHeader.UpdateImageIndex = UpdateImageIndex<br>
> FmpCapsuleImageHeader.Payload = Payload<br>
> FmpCapsuleImageHeader.VendorCodeBytes = VendorCodeBytes<br>
> FmpCapsuleImageHeader.UpdateHardwareInstance = HardwareInstance<br>
> + FmpCapsuleImageHeader.ImageCapsuleSupport = ImageCapsuleSupport<br>
> FmpCapsuleImage = FmpCapsuleImageHeader.Encode ()<br>
> FmpCapsuleData = FmpCapsuleData + FmpCapsuleImage<br>
> <br>
> --<br>
> 2.17.1<br>
> <br>
> <br>
> <br>
> <br>
> <u></u><u></u></p>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</blockquote></div></div>
<div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links:<p> You receive all messages sent to this group. <p> <a target="_blank" href="https://edk2.groups.io/g/devel/message/74346">View/Reply Online (#74346)</a> | | <a target="_blank" href="https://groups.io/mt/82206170/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a><br> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com]<br> <div width="1" style="color:white;clear:both">_._,_._,_</div>