Re: [PATCH] IntelFsp2WrapprPkg: Check header revision for MultiPhase support.

["Chiu, Chasel" <chasel.chiu@intel.com>]

Patch merged: https://github.com/tianocore/edk2/commit/26638d2aa399e436f47d629f877e581ed402020a

Thanks,
Chasel


> -----Original Message-----
> From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
> Sent: Tuesday, October 25, 2022 3:19 PM
> To: Chiu, Chasel <chasel.chiu@intel.com>; devel@edk2.groups.io
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] IntelFsp2WrapprPkg: Check header revision for
> MultiPhase support.
> 
> Hi Chasel,
> 
> There is a spelling error in your commit message. IntelFsp2WrapprPkg
> should be IntelFsp2WrapperPkg. Please fix this during the push. No need to
> send a 2nd review.
> 
> With that change...
> 
> Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
> 
> > -----Original Message-----
> > From: Chiu, Chasel <chasel.chiu@intel.com>
> > Sent: Tuesday, October 25, 2022 1:56 PM
> > To: devel@edk2.groups.io
> > Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: [PATCH] IntelFsp2WrapprPkg: Check header revision for
> > MultiPhase support.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4119
> >
> > Earlier version of FSP header may not have MultiPhase fields present
> > in the FspInfoHeader so the handler should verify header revision
> > before accessing the MultiPhase fields from the header.
> >
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> > ---
> >
> >
> IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWr
> app
> > erMultiPhaseProcessLib.c | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git
> >
> a/IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFsp
> Wra
> > pperMultiPhaseProcessLib.c
> >
> b/IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFsp
> Wra
> > pperMultiPhaseProcessLib.c
> > index 1248accf87..0f524910d5 100644
> > ---
> >
> a/IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFsp
> Wra
> > pperMultiPhaseProcessLib.c
> > +++
> b/IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFs
> > +++ pWrapperMultiPhaseProcessLib.c
> > @@ -73,15 +73,27 @@ CallFspMultiPhaseEntry (
> >    //
> >    // FSP_MULTI_PHASE_INIT and FSP_MULTI_PHASE_SI_INIT API
> functions having same prototype.
> >    //
> > -  UINTN       FspMultiPhaseApiEntry;
> > -  UINTN       FspMultiPhaseApiOffset;
> > -  EFI_STATUS  Status;
> > -  BOOLEAN     InterruptState;
> > +  UINTN                   FspMultiPhaseApiEntry;
> > +  UINTN                   FspMultiPhaseApiOffset;
> > +  EFI_STATUS              Status;
> > +  BOOLEAN                 InterruptState;
> > +  BOOLEAN                 IsVariableServiceRequest;
> > +  FSP_MULTI_PHASE_PARAMS  *FspMultiPhaseParamsPtr;
> > +
> > +  FspMultiPhaseParamsPtr   = (FSP_MULTI_PHASE_PARAMS
> *)FspMultiPhaseParams;
> > +  IsVariableServiceRequest = FALSE;
> > +  if ((FspMultiPhaseParamsPtr->MultiPhaseAction ==
> EnumMultiPhaseGetVariableRequestInfo) ||
> > +      (FspMultiPhaseParamsPtr->MultiPhaseAction ==
> > + EnumMultiPhaseCompleteVariableRequest))
> > +  {
> > +    IsVariableServiceRequest = TRUE;
> > +  }
> >
> >    if (ComponentIndex == FspMultiPhaseMemInitApiIndex) {
> >      FspHeader = (FSP_INFO_HEADER *)FspFindFspHeader (PcdGet32
> (PcdFspmBaseAddress));
> >      if (FspHeader == NULL) {
> >        return EFI_DEVICE_ERROR;
> > +    } else if (FspHeader->SpecVersion < 0x24) {
> > +      return EFI_UNSUPPORTED;
> >      }
> >
> >      FspMultiPhaseApiOffset =
> > FspHeader->FspMultiPhaseMemInitEntryOffset;
> > @@ -89,6 +101,10 @@ CallFspMultiPhaseEntry (
> >      FspHeader = (FSP_INFO_HEADER *)FspFindFspHeader (PcdGet32
> (PcdFspsBaseAddress));
> >      if (FspHeader == NULL) {
> >        return EFI_DEVICE_ERROR;
> > +    } else if (FspHeader->SpecVersion < 0x22) {
> > +      return EFI_UNSUPPORTED;
> > +    } else if ((FspHeader->SpecVersion < 0x24) &&
> (IsVariableServiceRequest == TRUE)) {
> > +      return EFI_UNSUPPORTED;
> >      }
> >
> >      FspMultiPhaseApiOffset =
> > FspHeader->FspMultiPhaseSiInitEntryOffset;
> > @@ -120,7 +136,8 @@ CallFspMultiPhaseEntry (
> >    @param[in] FspHobListPtr        - Pointer to FSP HobList (valid after FSP-
> M completed)
> >    @param[in] ComponentIndex       - FSP Component which executing
> MultiPhase initialization.
> >
> > -  @retval EFI_UNSUPPORTED   FSP Wrapper cannot support the specific
> variable request
> > +  @retval EFI_UNSUPPORTED   FSP Wrapper cannot support the specific
> variable request,
> > +                            or FSP does not support VariableService
> >    @retval EFI_STATUS        Return FSP returned status
> >
> >  **/
> > @@ -287,7 +304,8 @@ FspWrapperVariableRequestHandler (
> >    @param[in] FspHobListPtr        - Pointer to FSP HobList (valid after FSP-
> M completed)
> >    @param[in] ComponentIndex       - FSP Component which executing
> MultiPhase initialization.
> >
> > -  @retval EFI_STATUS        Always return EFI_SUCCESS
> > +  @retval EFI_UNSUPPORTED   Specific MultiPhase action was not
> supported.
> > +  @retval EFI_SUCCESS       MultiPhase action were completed
> successfully.
> >
> >  **/
> >  EFI_STATUS
> > --
> > 2.35.0.windows.1