Re: [edk2-devel] [PATCH V4 5/6] MdeModulePkg: Add FMP Capsule Image Header extension

["Xu, Wei6" <wei6.xu@intel.com>]

Thanks for helping update the related version check.
Please see my comment below.
With these two places updated, Reviewed-by: Wei6 Xu <wei6.xu@intel.com>

>-----Original Message-----
>From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oleksiy
>Yakovlev
>Sent: Wednesday, May 13, 2020 6:06 AM
>To: devel@edk2.groups.io
>Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
>A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
><chao.b.zhang@intel.com>; Felixp@ami.com; oleksiyy@ami.com
>Subject: [edk2-devel] [PATCH V4 5/6] MdeModulePkg: Add FMP Capsule Image
>Header extension
>
>Add bitmask to structure which gives a binary-inspectable mechanism to
>determine if a capsule contains an authentication section or depex section.
>(UEFI 2.8 errata a, mantis 2026)
>
>Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
>---
> MdeModulePkg/Application/CapsuleApp/CapsuleDump.c    |  1 +
> .../Library/DxeCapsuleLibFmp/DxeCapsuleLib.c         | 20
>++++++++++++++++----
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
>diff --git a/MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
>b/MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
>index 7e3e072..e3ab199 100644
>--- a/MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
>+++ b/MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
>@@ -98,6 +98,7 @@ DumpFmpCapsule (
>     Print(L"  UpdateVendorCodeSize   - 0x%x\n",
>FmpImageHeader->UpdateVendorCodeSize);
>     if (FmpImageHeader->Version >=
>EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION) {
>       Print(L"  UpdateHardwareInstance - 0x%lx\n",
>FmpImageHeader->UpdateHardwareInstance);
>+      Print(L"  ImageCapsuleSupport - 0x%lx\n",
>+ FmpImageHeader->ImageCapsuleSupport);
>     }
>   }
> }

Wei: I think here the version check should be:
     if (ImageHeader->Version >= 1) {
       DEBUG((DEBUG_VERBOSE, "    UpdateHardwareInstance - 0x%lx\n", ImageHeader->UpdateHardwareInstance));
       if (ImageHeader->Version >= EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION) {
         DEBUG((DEBUG_VERBOSE, "    ImageCapsuleSupport - 0x%lx\n", ImageHeader->ImageCapsuleSupport));
       }
     }

>diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
>b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
>index 5dda561..f9819a7 100644
>--- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
>+++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
>@@ -285,8 +285,10 @@ ValidateFmpCapsule (
>       DEBUG((DEBUG_ERROR, "ImageHeader->Version(0x%x) Unknown\n",
>ImageHeader->Version));
>       return EFI_INVALID_PARAMETER;
>     }
>-    if (ImageHeader->Version <
>EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION) {
>+    if (ImageHeader->Version == 1) {
>       FmpImageHeaderSize =
>OFFSET_OF(EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER,
>UpdateHardwareInstance);
>+    } else {
>+      FmpImageHeaderSize =
>+ OFFSET_OF(EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER,
>+ ImageCapsuleSupport);
>     }
>     if (FmpImageSize < FmpImageHeaderSize) {
>       DEBUG((DEBUG_ERROR, "FmpImageSize(0x%lx) <
>FmpImageHeaderSize(0x%x)\n", FmpImageSize, FmpImageHeaderSize)); @@
>-521,6 +523,7 @@ DumpFmpCapsule (
>     DEBUG((DEBUG_VERBOSE, "    UpdateVendorCodeSize   - 0x%x\n",
>ImageHeader->UpdateVendorCodeSize));
>     if (ImageHeader->Version >=
>EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION) {
>       DEBUG((DEBUG_VERBOSE, "    UpdateHardwareInstance - 0x%lx\n",
>ImageHeader->UpdateHardwareInstance));
>+      DEBUG((DEBUG_VERBOSE, "    ImageCapsuleSupport - 0x%lx\n",
>ImageHeader->ImageCapsuleSupport));
>     }
>   }
> }

Wei: I think here the version check should be:
   if (ImageHeader->Version >= 1) {
     DEBUG((DEBUG_INFO, "(UpdateHardwareInstance - 0x%x)", ImageHeader->UpdateHardwareInstance));
     if (ImageHeader->Version >= EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION) {
       DEBUG((DEBUG_INFO, "(ImageCapsuleSupport - 0x%x)",  ImageHeader->ImageCapsuleSupport));
     }
   }


>@@ -928,9 +931,14 @@ SetFmpImageData (
>   } else {
>     //
>     // If the EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER is
>version 1,
>-    // Header should exclude UpdateHardwareInstance field
>+    // Header should exclude UpdateHardwareInstance field, and
>+    // ImageCapsuleSupport field if version is 2.
>     //
>-    Image = (UINT8 *)ImageHeader +
>OFFSET_OF(EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER,
>UpdateHardwareInstance);
>+    if (ImageHeader->Version == 1) {
>+      Image = (UINT8 *)ImageHeader +
>OFFSET_OF(EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER,
>UpdateHardwareInstance);
>+    } else {
>+      Image = (UINT8 *)ImageHeader +
>OFFSET_OF(EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER,
>ImageCapsuleSupport);
>+    }
>   }
>
>   if (ImageHeader->UpdateVendorCodeSize == 0) { @@ -945,6 +953,7 @@
>SetFmpImageData (
>   DEBUG((DEBUG_INFO, "ImageIndex - 0x%x ",
>ImageHeader->UpdateImageIndex));
>   if (ImageHeader->Version >=
>EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION) {
>     DEBUG((DEBUG_INFO, "(UpdateHardwareInstance - 0x%x)",
>ImageHeader->UpdateHardwareInstance));
>+    DEBUG((DEBUG_INFO, "(ImageCapsuleSupport - 0x%x)",
>+ ImageHeader->ImageCapsuleSupport));
>   }
>   DEBUG((DEBUG_INFO, "\n"));
>
>@@ -1239,7 +1248,10 @@ ProcessFmpCapsuleImage (
>     ImageHeader  =
>(EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER *)((UINT8
>*)FmpCapsuleHeader + ItemOffsetList[Index]);
>
>     UpdateHardwareInstance = 0;
>-    if (ImageHeader->Version >=
>EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION) {
>+    ///
>+    /// UpdateHardwareInstance field was added in Version 2
>+    ///
>+    if (ImageHeader->Version >= 2) {
>       UpdateHardwareInstance = ImageHeader->UpdateHardwareInstance;
>     }
>
>--
>2.9.0.windows.1
>
>
>Please consider the environment before printing this email.
>
>The information contained in this message may be confidential and proprietary
>to American Megatrends (AMI).  This communication is intended to be read
>only by the individual or entity to whom it is addressed or by their designee. If
>the reader of this message is not the intended recipient, you are on notice that
>any distribution of this message, in any form, is strictly prohibited.  Please
>promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and
>then delete or destroy all copies of the transmission.
>
>