Re: [PATCH] IntelFsp2Pkg: Support to return error status from FSP API done

["Mudusuru, Giri P" <giri.p.mudusuru@intel.com>]

Hi Richard,

In the below snippet the debug message is based on the API status and will not be display if the switch stack already happened right? Also since it is in a do while loop "cannot proceed further" is not necessarily accurate.

> -  SetFspApiReturnStatus (EFI_SUCCESS);
> -
> -  Pei2LoaderSwitchStack();
> -
> +  do {
> +    SetFspApiReturnStatus (Status);
> +    Pei2LoaderSwitchStack ();
> +    if (Status != EFI_SUCCESS) {
> +      DEBUG ((DEBUG_ERROR, "!!!ERROR: FspSiliconInitApi() - [Status: 0x%08X] -
> Error encountered during previous API and cannot proceed further\n", Status));
> +    }
> +  } while (Status != EFI_SUCCESS);
Thanks,
-Giri

> -----Original Message-----
> From: Thomaiyar, Richard Marian
> Sent: Tuesday, November 8, 2016 1:45 AM
> To: edk2-devel@lists.01.org
> Cc: Ma, Maurice <maurice.ma@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Mudusuru, Giri P <giri.p.mudusuru@intel.com>;
> Thomaiyar, Richard Marian <richard.marian.thomaiyar@intel.com>
> Subject: [PATCH] IntelFsp2Pkg: Support to return error status from FSP API done
> 
> Added FspMemoryInitDone2, FspTempRamExitDone2, FspSiliconInitDone2
> to return error status to Boot Loader for FSP API calls.
> To maintain backward compatibility existing functions
> (FspMemoryInitDone, FspTempRamExitDone, FspSiliconInitDone)
> declaration left untouched.
> 
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Giri P Mudusuru <giri.p.mudusuru@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Richard Thomaiyar <richard.marian.thomaiyar@intel.com>
> ---
>  IntelFsp2Pkg/Include/Library/FspPlatformLib.h      |  35 +++++
>  .../Library/BaseFspPlatformLib/FspPlatformNotify.c | 151 +++++++++++++++++-
> ---
>  2 files changed, 163 insertions(+), 23 deletions(-)
> 
> diff --git a/IntelFsp2Pkg/Include/Library/FspPlatformLib.h
> b/IntelFsp2Pkg/Include/Library/FspPlatformLib.h
> index 61e77bd..1f39601 100644
> --- a/IntelFsp2Pkg/Include/Library/FspPlatformLib.h
> +++ b/IntelFsp2Pkg/Include/Library/FspPlatformLib.h
> @@ -93,4 +93,39 @@ FspWaitForNotify (
>    VOID
>    );
> 
> +/**
> +  This function transfer control back to BootLoader after FspSiliconInit.
> +
> +  @param[in] Status return status for the FspSiliconInit.
> +**/
> +VOID
> +EFIAPI
> +FspSiliconInitDone2 (
> +  IN EFI_STATUS Status
> +  );
> +
> +/**
> +  This function returns control to BootLoader after MemoryInitApi.
> +
> +  @param[in] Status return status for the MemoryInitApi.
> +  @param[in,out] HobListPtr The address of HobList pointer.
> +**/
> +VOID
> +EFIAPI
> +FspMemoryInitDone2 (
> +  IN EFI_STATUS Status,
> +  IN OUT VOID   **HobListPtr
> +  );
> +
> +/**
> +  This function returns control to BootLoader after TempRamExitApi.
> +
> +  @param[in] Status return status for the TempRamExitApi.
> +**/
> +VOID
> +EFIAPI
> +FspTempRamExitDone2 (
> +  IN EFI_STATUS Status
> +  );
> +
>  #endif
> diff --git a/IntelFsp2Pkg/Library/BaseFspPlatformLib/FspPlatformNotify.c
> b/IntelFsp2Pkg/Library/BaseFspPlatformLib/FspPlatformNotify.c
> index 755e84f..ac1fc1a 100644
> --- a/IntelFsp2Pkg/Library/BaseFspPlatformLib/FspPlatformNotify.c
> +++ b/IntelFsp2Pkg/Library/BaseFspPlatformLib/FspPlatformNotify.c
> @@ -108,35 +108,56 @@ FspNotificationHandler (
>  /**
>    This function transfer control back to BootLoader after FspSiliconInit.
> 
> +  @param[in] Status return status for the FspSiliconInit.
> +
>  **/
>  VOID
>  EFIAPI
> -FspSiliconInitDone (
> -  VOID
> +FspSiliconInitDone2 (
> +  IN EFI_STATUS Status
>    )
>  {
>    //
> +  // Convert to FSP EAS defined API return codes
> +  //
> +  switch (Status) {
> +    case EFI_SUCCESS:
> +    case EFI_INVALID_PARAMETER:
> +    case EFI_UNSUPPORTED:
> +    case EFI_DEVICE_ERROR:
> +      break;
> +    default:
> +      DEBUG ((DEBUG_INFO | DEBUG_INIT, "FspSiliconInitApi() Invalid Error -
> [Status: 0x%08X]\n", Status));
> +      Status = EFI_DEVICE_ERROR;  // Force to known error.
> +      break;
> +  }
> +  //
>    // This is the end of the FspSiliconInit API
>    // Give control back to the boot loader
>    //
>    SetFspMeasurePoint (FSP_PERF_ID_API_FSP_SILICON_INIT_EXIT);
> -  DEBUG ((DEBUG_INFO | DEBUG_INIT, "FspSiliconInitApi() - End\n"));
> +  DEBUG ((DEBUG_INFO | DEBUG_INIT, "FspSiliconInitApi() - [Status: 0x%08X] -
> End\n", Status));
>    PERF_END_EX (&gFspPerformanceDataGuid, "EventRec", NULL, 0,
> FSP_STATUS_CODE_SILICON_INIT | FSP_STATUS_CODE_COMMON_CODE |
> FSP_STATUS_CODE_API_EXIT);
>    REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
> FSP_STATUS_CODE_COMMON_CODE | FSP_STATUS_CODE_API_EXIT);
> -  SetFspApiReturnStatus (EFI_SUCCESS);
> -
> -  Pei2LoaderSwitchStack();
> -
> +  do {
> +    SetFspApiReturnStatus (Status);
> +    Pei2LoaderSwitchStack ();
> +    if (Status != EFI_SUCCESS) {
> +      DEBUG ((DEBUG_ERROR, "!!!ERROR: FspSiliconInitApi() - [Status: 0x%08X] -
> Error encountered during previous API and cannot proceed further\n", Status));
> +    }
> +  } while (Status != EFI_SUCCESS);
>  }
> 
>  /**
>    This function returns control to BootLoader after MemoryInitApi.
> 
> -  @param[in,out] HobListPtr The address of HobList pointer.
> +  @param[in] Status return status for the MemoryInitApi.
> +  @param[in,out] HobListPtr The address of HobList pointer, if NULL, will get
> value from GetFspApiParameter2 ()
>  **/
>  VOID
>  EFIAPI
> -FspMemoryInitDone (
> +FspMemoryInitDone2 (
> +  IN EFI_STATUS Status,
>    IN OUT VOID   **HobListPtr
>    )
>  {
> @@ -145,15 +166,32 @@ FspMemoryInitDone (
>    // Calling use FspMemoryInit API
>    // Update HOB and return the control directly
>    //
> +  if (HobListPtr == NULL) {
> +    HobListPtr = (VOID **)GetFspApiParameter2 ();
> +  }
>    if (HobListPtr != NULL) {
>      *HobListPtr = (VOID *) GetHobList ();
>    }
> -
> +  //
> +  // Convert to FSP EAS defined API return codes
> +  //
> +  switch (Status) {
> +    case EFI_SUCCESS:
> +    case EFI_INVALID_PARAMETER:
> +    case EFI_UNSUPPORTED:
> +    case EFI_DEVICE_ERROR:
> +    case EFI_OUT_OF_RESOURCES:
> +      break;
> +    default:
> +      DEBUG ((DEBUG_INFO | DEBUG_INIT, "FspMemoryInitApi() Invalid Error
> [Status: 0x%08X]\n", Status));
> +      Status = EFI_DEVICE_ERROR;  // Force to known error.
> +      break;
> +  }
>    //
>    // This is the end of the FspMemoryInit API
>    // Give control back to the boot loader
>    //
> -  DEBUG ((DEBUG_INFO | DEBUG_INIT, "FspMemoryInitApi() - End\n"));
> +  DEBUG ((DEBUG_INFO | DEBUG_INIT, "FspMemoryInitApi() - [Status: 0x%08X]
> - End\n", Status));
>    SetFspMeasurePoint (FSP_PERF_ID_API_FSP_MEMORY_INIT_EXIT);
>    FspData = GetFspGlobalDataPointer ();
>    PERF_START_EX(&gFspPerformanceDataGuid, "EventRec", NULL, (FspData-
> >PerfData[0] & FSP_PERFORMANCE_DATA_TIMER_MASK),
> FSP_STATUS_CODE_TEMP_RAM_INIT | FSP_STATUS_CODE_COMMON_CODE|
> FSP_STATUS_CODE_API_ENTRY);
> @@ -161,8 +199,13 @@ FspMemoryInitDone (
>    PERF_START_EX(&gFspPerformanceDataGuid, "EventRec", NULL, (FspData-
> >PerfData[2] & FSP_PERFORMANCE_DATA_TIMER_MASK),
> FSP_STATUS_CODE_MEMORY_INIT | FSP_STATUS_CODE_COMMON_CODE |
> FSP_STATUS_CODE_API_ENTRY);
>    PERF_END_EX(&gFspPerformanceDataGuid, "EventRec", NULL, 0,
> FSP_STATUS_CODE_MEMORY_INIT | FSP_STATUS_CODE_COMMON_CODE |
> FSP_STATUS_CODE_API_EXIT);
>    REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
> FSP_STATUS_CODE_MEMORY_INIT | FSP_STATUS_CODE_COMMON_CODE |
> FSP_STATUS_CODE_API_EXIT);
> -  SetFspApiReturnStatus (EFI_SUCCESS);
> -  Pei2LoaderSwitchStack ();
> +  do {
> +    SetFspApiReturnStatus (Status);
> +    Pei2LoaderSwitchStack ();
> +    if (Status != EFI_SUCCESS) {
> +      DEBUG ((DEBUG_ERROR, "!!!ERROR: FspMemoryInitApi() - [Status: 0x%08X]
> - Error encountered during previous API and cannot proceed further\n", Status));
> +    }
> +  } while (Status != EFI_SUCCESS);
> 
>    //
>    // The TempRamExitApi is called
> @@ -185,25 +228,44 @@ FspMemoryInitDone (
>  /**
>    This function returns control to BootLoader after TempRamExitApi.
> 
> +  @param[in] Status return status for the TempRamExitApi.
> +
>  **/
>  VOID
>  EFIAPI
> -FspTempRamExitDone (
> -  VOID
> +FspTempRamExitDone2 (
> +  IN EFI_STATUS Status
>    )
>  {
> -
> +  //
> +  // Convert to FSP EAS defined API return codes
> +  //
> +  switch (Status) {
> +    case EFI_SUCCESS:
> +    case EFI_INVALID_PARAMETER:
> +    case EFI_UNSUPPORTED:
> +    case EFI_DEVICE_ERROR:
> +      break;
> +    default:
> +      DEBUG ((DEBUG_INFO | DEBUG_INIT, "TempRamExitApi() Invalid Error -
> [Status: 0x%08X]\n", Status));
> +      Status = EFI_DEVICE_ERROR;  // Force to known error.
> +      break;
> +  }
>    //
>    // This is the end of the TempRamExit API
>    // Give control back to the boot loader
>    //
> -  DEBUG ((DEBUG_INFO | DEBUG_INIT, "TempRamExitApi() - End\n"));
> +  DEBUG ((DEBUG_INFO | DEBUG_INIT, "TempRamExitApi() - [Status: 0x%08X]
> - End\n", Status));
>    SetFspMeasurePoint (FSP_PERF_ID_API_TEMP_RAM_EXIT_EXIT);
>    PERF_END_EX(&gFspPerformanceDataGuid, "EventRec", NULL, 0,
> FSP_STATUS_CODE_TEMP_RAM_EXIT | FSP_STATUS_CODE_COMMON_CODE |
> FSP_STATUS_CODE_API_EXIT);
>    REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
> FSP_STATUS_CODE_TEMP_RAM_EXIT | FSP_STATUS_CODE_COMMON_CODE |
> FSP_STATUS_CODE_API_EXIT);
> -  SetFspApiReturnStatus (EFI_SUCCESS);
> -  Pei2LoaderSwitchStack ();
> -
> +  do {
> +    SetFspApiReturnStatus (Status);
> +    Pei2LoaderSwitchStack ();
> +    if (Status != EFI_SUCCESS) {
> +      DEBUG ((DEBUG_ERROR, "!!!ERROR: TempRamExitApi() - [Status: 0x%08X] -
> Error encountered during previous API and cannot proceed further\n", Status));
> +    }
> +  } while (Status != EFI_SUCCESS);
>    SetPhaseStatusCode (FSP_STATUS_CODE_SILICON_INIT);
>    SetFspMeasurePoint (FSP_PERF_ID_API_FSP_SILICON_INIT_ENTRY);
>    PERF_START_EX(&gFspPerformanceDataGuid, "EventRec", NULL, 0,
> FSP_STATUS_CODE_SILICON_INIT | FSP_STATUS_CODE_COMMON_CODE |
> FSP_STATUS_CODE_API_ENTRY);
> @@ -266,9 +328,7 @@ FspWaitForNotify (
>        }
>      }
> 
> -    SetFspApiReturnStatus(Status);
>      DEBUG ((DEBUG_INFO | DEBUG_INIT, "NotifyPhaseApi() - End  [Status:
> 0x%08X]\n", Status));
> -
>      SetFspMeasurePoint (FSP_PERF_ID_API_NOTIFY_POST_PCI_EXIT + Count);
> 
>      if ((NotificationCount - 1) == 0) {
> @@ -281,7 +341,13 @@ FspWaitForNotify (
>        PERF_END_EX(&gFspPerformanceDataGuid, "EventRec", NULL, 0,
> FSP_STATUS_CODE_END_OF_FIRMWARE_NOTIFICATION |
> FSP_STATUS_CODE_COMMON_CODE | FSP_STATUS_CODE_API_EXIT);
>        REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
> FSP_STATUS_CODE_END_OF_FIRMWARE_NOTIFICATION |
> FSP_STATUS_CODE_COMMON_CODE | FSP_STATUS_CODE_API_EXIT);
>      }
> -    Pei2LoaderSwitchStack();
> +    do {
> +      SetFspApiReturnStatus(Status);
> +      Pei2LoaderSwitchStack();
> +      if (Status != EFI_SUCCESS) {
> +        DEBUG ((DEBUG_ERROR, "!!!ERROR: NotifyPhaseApi() [Phase: %08X] -
> Failed - [Status: 0x%08X]\n", NotificationValue, Status));
> +      }
> +    } while (Status != EFI_SUCCESS);
>    }
> 
>    //
> @@ -290,3 +356,42 @@ FspWaitForNotify (
>    //
>  }
> 
> +/**
> +  This function transfer control back to BootLoader after FspSiliconInit.
> +
> +**/
> +VOID
> +EFIAPI
> +FspSiliconInitDone (
> +  VOID
> +  )
> +{
> +  FspSiliconInitDone2 (EFI_SUCCESS);
> +}
> +
> +/**
> +  This function returns control to BootLoader after MemoryInitApi.
> +
> +  @param[in,out] HobListPtr The address of HobList pointer.
> +**/
> +VOID
> +EFIAPI
> +FspMemoryInitDone (
> +  IN OUT VOID   **HobListPtr
> +  )
> +{
> +  FspMemoryInitDone2 (EFI_SUCCESS, HobListPtr);
> +}
> +
> +/**
> +  This function returns control to BootLoader after TempRamExitApi.
> +
> +**/
> +VOID
> +EFIAPI
> +FspTempRamExitDone (
> +  VOID
> +  )
> +{
> +  FspTempRamExitDone2 (EFI_SUCCESS);
> +}
> --
> 2.9.0.windows.1