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

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

Thanks Richard. Can you please add comments in the code to describe it? Other than that it looks good to me.

Reviewed-by: Giri P Mudusuru <giri.p.mudusuru@intel.com>


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Mudusuru, Giri P
> Sent: Tuesday, November 8, 2016 2:39 PM
> To: Thomaiyar, Richard Marian <richard.marian.thomaiyar@intel.com>; edk2-
> devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel