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

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

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/PeiFspWrapperMultiPhaseProcessLib.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c b/IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c
> index 1248accf87..0f524910d5 100644
> --- a/IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c
> +++ b/IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.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