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

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 15 Dec 2022 at 04:26, Xu, Min M <min.m.xu@intel.com> wrote:
>
> 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.
>

I have no objections to this

Acked-by: Ard Biesheuvel <ardb@kernel.org>

>
> > -----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
>