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

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: Schaefer, Daniel
> Sent: Friday, September 11, 2020 4:01 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io
> Cc: Leif Lindholm <leif@nuviainc.com>
> Subject: Re: [edk2-platforms][PATCH 2/2] Silicon/SiFive: Error handling for
> locating firmware context
> 
> 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.
That's fine to me, Daniel. Just don’t let process keep going with an unexpected error. Now you got the ownership of this issue, just send out your patch for review.
Thanks
> 
> > +    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) {
> >