Re: [PATCH V2 1/4] EmbeddedPkg/PrePiLib: Add FFS_CHECK_SECTION_HOOK when finding section

["Min Xu" <min.m.xu@intel.com>]

Hi, Leif/Ard/Abner/Daniel
Since you're the maintainer/reviewer of EmbeddedPkg, would you please help to review this patch? Any comments is welcome.

Thanks
Min

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Tuesday, December 13, 2022 2:24 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Abner Chang <abner.chang@amd.com>; Daniel Schaefer
> <git@danielschaefer.me>; Gerd Hoffmann <kraxel@redhat.com>; Aktas,
> Erdem <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: [PATCH V2 1/4] EmbeddedPkg/PrePiLib: Add
> FFS_CHECK_SECTION_HOOK when finding section
> 
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4152
> 
> EmbeddedPkg/PrePiLib provides the service of finding sections based on the
> input SectionType. But sometimes there maybe multiple sections with the
> same SectionType. FFS_CHECK_SECTION_HOOK is a hook which can be called
> to do additional check.
> 
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Daniel Schaefer <git@danielschaefer.me>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> 
> EmbeddedPkg
> ---
>  EmbeddedPkg/Include/Library/PrePiLib.h  | 23 +++++++++++---
>  EmbeddedPkg/Library/PrePiLib/FwVol.c    | 42 ++++++++++++++++++-------
>  EmbeddedPkg/Library/PrePiLib/PrePiLib.c |  2 +-
>  3 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/EmbeddedPkg/Include/Library/PrePiLib.h
> b/EmbeddedPkg/Include/Library/PrePiLib.h
> index 3741b08c4478..f60b6678185a 100644
> --- a/EmbeddedPkg/Include/Library/PrePiLib.h
> +++ b/EmbeddedPkg/Include/Library/PrePiLib.h
> @@ -52,11 +52,23 @@ FfsFindNextFile (
>    IN OUT EFI_PEI_FILE_HANDLE  *FileHandle
>    );
> 
> +/**
> + * This is a hook which is used to check if the section is the target one.
> + *
> + */
> +typedef
> +EFI_STATUS
> +(EFIAPI *FFS_CHECK_SECTION_HOOK)(
> +  IN EFI_COMMON_SECTION_HEADER *Section
> +  );
> +
>  /**
>    This service enables discovery sections of a given type within a valid FFS file.
> +  Caller also can provide a SectionCheckHook to do additional checking.
> 
> -  @param  SearchType            The value of the section type to find.
> -  @param  FfsFileHeader         A pointer to the file header that contains the
> set of sections to
> +  @param  SectionType           The value of the section type to find.
> +  @param  SectionCheckHook      A hook which can check if the section is the
> target one.
> +  @param  FileHeader            A pointer to the file header that contains the set
> of sections to
>                                  be searched.
>    @param  SectionData           A pointer to the discovered section, if successful.
> 
> @@ -67,9 +79,10 @@ FfsFindNextFile (
>  EFI_STATUS
>  EFIAPI
>  FfsFindSectionData (
> -  IN EFI_SECTION_TYPE     SectionType,
> -  IN EFI_PEI_FILE_HANDLE  FileHandle,
> -  OUT VOID                **SectionData
> +  IN EFI_SECTION_TYPE        SectionType,
> +  IN FFS_CHECK_SECTION_HOOK  SectionCheckHook,
> +  IN EFI_PEI_FILE_HANDLE     FileHandle,
> +  OUT VOID                   **SectionData
>    );
> 
>  /**
> diff --git a/EmbeddedPkg/Library/PrePiLib/FwVol.c
> b/EmbeddedPkg/Library/PrePiLib/FwVol.c
> index 0a6d6925b7ea..778d8b13c33b 100644
> --- a/EmbeddedPkg/Library/PrePiLib/FwVol.c
> +++ b/EmbeddedPkg/Library/PrePiLib/FwVol.c
> @@ -264,16 +264,18 @@ FindFileEx (
>    Go through the file to search SectionType section,
>    when meeting an encapsuled section.
> 
> -  @param  SectionType  - Filter to find only section of this type.
> -  @param  Section      - From where to search.
> -  @param  SectionSize  - The file size to search.
> -  @param  OutputBuffer - Pointer to the section to search.
> +  @param  SectionType       - Filter to find only section of this type.
> +  @param  SectionCheckHook  - A hook which can check if the section is the
> target one.
> +  @param  Section           - From where to search.
> +  @param  SectionSize       - The file size to search.
> +  @param  OutputBuffer      - Pointer to the section to search.
> 
>    @retval EFI_SUCCESS
>  **/
>  EFI_STATUS
>  FfsProcessSection (
>    IN EFI_SECTION_TYPE           SectionType,
> +  IN FFS_CHECK_SECTION_HOOK     SectionCheckHook,
>    IN EFI_COMMON_SECTION_HEADER  *Section,
>    IN UINTN                      SectionSize,
>    OUT VOID                      **OutputBuffer
> @@ -292,7 +294,9 @@ FfsProcessSection (
>    UINT32                    AuthenticationStatus;
>    CHAR8                     *CompressedData;
>    UINT32                    CompressedDataLength;
> +  BOOLEAN                   Found;
> 
> +  Found         = FALSE;
>    *OutputBuffer = NULL;
>    ParsedLength  = 0;
>    Status        = EFI_NOT_FOUND;
> @@ -302,13 +306,23 @@ FfsProcessSection (
>      }
> 
>      if (Section->Type == SectionType) {
> -      if (IS_SECTION2 (Section)) {
> -        *OutputBuffer = (VOID *)((UINT8 *)Section + sizeof
> (EFI_COMMON_SECTION_HEADER2));
> +      if (SectionCheckHook != NULL) {
> +        Found = SectionCheckHook (Section) == EFI_SUCCESS;
>        } else {
> -        *OutputBuffer = (VOID *)((UINT8 *)Section + sizeof
> (EFI_COMMON_SECTION_HEADER));
> +        Found = TRUE;
>        }
> 
> -      return EFI_SUCCESS;
> +      if (Found) {
> +        if (IS_SECTION2 (Section)) {
> +          *OutputBuffer = (VOID *)((UINT8 *)Section + sizeof
> (EFI_COMMON_SECTION_HEADER2));
> +        } else {
> +          *OutputBuffer = (VOID *)((UINT8 *)Section + sizeof
> (EFI_COMMON_SECTION_HEADER));
> +        }
> +
> +        return EFI_SUCCESS;
> +      } else {
> +        goto CheckNextSection;
> +      }
>      } else if ((Section->Type == EFI_SECTION_COMPRESSION) || (Section-
> >Type == EFI_SECTION_GUID_DEFINED)) {
>        if (Section->Type == EFI_SECTION_COMPRESSION) {
>          if (IS_SECTION2 (Section)) {
> @@ -415,6 +429,7 @@ FfsProcessSection (
>        } else {
>          return FfsProcessSection (
>                   SectionType,
> +                 SectionCheckHook,
>                   DstBuffer,
>                   DstBufferSize,
>                   OutputBuffer
> @@ -422,6 +437,7 @@ FfsProcessSection (
>        }
>      }
> 
> +CheckNextSection:
>      if (IS_SECTION2 (Section)) {
>        SectionLength = SECTION2_SIZE (Section);
>      } else {
> @@ -456,9 +472,10 @@ FfsProcessSection (  EFI_STATUS  EFIAPI
> FfsFindSectionData (
> -  IN EFI_SECTION_TYPE     SectionType,
> -  IN EFI_PEI_FILE_HANDLE  FileHandle,
> -  OUT VOID                **SectionData
> +  IN EFI_SECTION_TYPE        SectionType,
> +  IN FFS_CHECK_SECTION_HOOK  SectionCheckHook,
> +  IN EFI_PEI_FILE_HANDLE     FileHandle,
> +  OUT VOID                   **SectionData
>    )
>  {
>    EFI_FFS_FILE_HEADER        *FfsFileHeader;
> @@ -478,6 +495,7 @@ FfsFindSectionData (
> 
>    return FfsProcessSection (
>             SectionType,
> +           SectionCheckHook,
>             Section,
>             FileSize,
>             SectionData
> @@ -799,7 +817,7 @@ FfsProcessFvFile (
>    //
>    // Find FvImage in FvFile
>    //
> -  Status = FfsFindSectionData (EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
> FvFileHandle, (VOID **)&FvImageHandle);
> +  Status = FfsFindSectionData (EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
> NULL,
> + FvFileHandle, (VOID **)&FvImageHandle);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> index a0c5d02debd0..3b6fc4f0eba8 100644
> --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> @@ -131,7 +131,7 @@ LoadDxeCoreFromFfsFile (
>    VOID                  *Hob;
>    EFI_FV_FILE_INFO      FvFileInfo;
> 
> -  Status = FfsFindSectionData (EFI_SECTION_PE32, FileHandle,
> &PeCoffImage);
> +  Status = FfsFindSectionData (EFI_SECTION_PE32, NULL, FileHandle,
> + &PeCoffImage);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> --
> 2.29.2.windows.2