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

["Thomaiyar, Richard Marian" <richard.marian.thomaiyar@intel.com>]

The debug message will be displayed only when FSP API returned error status, and when Boot Loader called the subsequent FSP API again. The idea here is to return the control back to Boot Loader if there is any error in the previous API Call, without proceeding further and not having a hang inside the FSP. 

Note: Regular FSP API call status will be displayed in debug message, before returning the control back to the Boot Loader. 
> +  DEBUG ((DEBUG_INFO | DEBUG_INIT, "FspSiliconInitApi() - [Status: 
> + 0x%08X] -
> End\n", Status));

Regards, 
Richard

-----Original Message-----
From: Mudusuru, Giri P 
Sent: Wednesday, November 9, 2016 4:09 AM
To: Thomaiyar, Richard Marian <richard.marian.thomaiyar@intel.com>; edk2-devel@lists.01.org
Cc: Ma, Maurice <maurice.ma@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH] IntelFsp2Pkg: Support to return error status from FSP API done

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