Re: [edk2-platforms][PATCH 2/2] Silicon/SiFive: Error handling for locating firmware context

["Daniel Schaefer" <daniel.schaefer@hpe.com>]

On 9/8/20 1:48 PM, Abner Chang wrote:
> Add error handling in CreateU54E51CoreProcessorSpecificDataHob ()
> when opensbi firmware context can't be located.
> 
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> ---
>   .../SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c   | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c b/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> index edeabf028f..ea947a9d28 100644
> --- a/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> +++ b/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> @@ -38,7 +38,7 @@
>   
>     @return EFI_SUCCESS     The PEIM initialized successfully.
>             EFI_UNSUPPORTED HART is ignored by platform.
> -
> +          EFI_NOT_FOUND   Processor specific data hob is not available.
>   **/
>   EFI_STATUS
>   EFIAPI
> @@ -64,9 +64,14 @@ CreateU54E51CoreProcessorSpecificDataHob (
>       return EFI_INVALID_PARAMETER;
>     }
>   
> +  FirmwareContext = NULL;
>     Status = SbiGetFirmwareContext (&FirmwareContext);
>     ASSERT_EFI_ERROR (Status);
> -  DEBUG ((DEBUG_INFO, "    Firmware Context is at 0x%x.\n", FirmwareContext));
> +  if (EFI_ERROR (Status) || FirmwareContext == NULL) {

The assert should already catch this, right? Especially if we fix the return value for
SbiGetFirmwareContext we don't also need to check the pointer.
I'd say: Let's fix the return value (like your other patch) and remove assert,
as well as the null-check here.

> +    DEBUG ((DEBUG_ERROR, "Failed to get the pointer of EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT of hart %d\n", HartId));
> +    return Status;
> +  }
> +  DEBUG((DEBUG_INFO, "    Firmware Context is at 0x%x.\n", FirmwareContext));
>     FirmwareContextHartSpecific = FirmwareContext->HartSpecific[HartId];
>     DEBUG ((DEBUG_INFO, "    Firmware Context Hart specific is at 0x%x.\n", FirmwareContextHartSpecific));
>     if (FirmwareContextHartSpecific == NULL) {
>