Re: [PATCH edk2-platforms 1/1] Silicon/SiFive: fix RELEASE builds of U54 PeiCoreInfoHobLib

["Leif Lindholm" <leif@nuviainc.com>]

On Sun, Aug 30, 2020 at 08:14:36 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif@nuviainc.com]
> > Sent: Saturday, August 29, 2020 9:41 PM
> > To: devel@edk2.groups.io
> > Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> > Chen, Gilbert <gilbert.chen@hpe.com>; Schaefer, Daniel
> > <daniel.schaefer@hpe.com>
> > Subject: [PATCH edk2-platforms 1/1] Silicon/SiFive: fix RELEASE builds of U54
> > PeiCoreInfoHobLib
> > 
> > In function CreateU54E51CoreProcessorSpecificDataHob(), the
> > FirmwareContext variable gets initialized an ASSERT_EFI_ERROR macro,
> > meaning the initialization gets folded out for RELEASE builds. Use a
> > temporary variable for the ASSERT instead.
> > 
> > Fixes a build error of both u540 and u500 with gcc 8.3.
> > 
> > Cc: Abner Chang <abner.chang@hpe.com>
> > Cc: Gilbert Chen <gilbert.chen@hpe.com>
> > Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >  Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> > b/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> > index 6ddae632fd17..edeabf028ff8 100644
> > --- a/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> > +++ b/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> > @@ -56,6 +56,7 @@ CreateU54E51CoreProcessorSpecificDataHob (
> >    RISC_V_PROCESSOR_SPECIFIC_HOB_DATA ProcessorSpecDataHob;
> >    EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *FirmwareContext;
> >    EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC
> > *FirmwareContextHartSpecific;
> > +  EFI_STATUS Status;
> > 
> >    DEBUG ((DEBUG_INFO, "%a: Entry.\n", __FUNCTION__));
> > 
> > @@ -63,7 +64,8 @@ CreateU54E51CoreProcessorSpecificDataHob (
> >      return EFI_INVALID_PARAMETER;
> >    }
> > 
> > -  ASSERT_EFI_ERROR (SbiGetFirmwareContext (&FirmwareContext));
> > +  Status = SbiGetFirmwareContext (&FirmwareContext);
> > ASSERT_EFI_ERROR
> ASSERT_EFI_ERROR() should start at the new line. I see this macro
> just attached to the end of Status = SbiGetFirmwareContext
> (&FirmwareContext). Is my editor problem?

https://edk2.groups.io/g/devel/message/64794
It's your editor.

> > + (Status);
> Please add error condition check for the Status. Return
> EFI_INVALID_PARAMETERS to caller, and also update the function
> header for the case of @retval is EFI_INVALID_PARAMETER. Just
> realized that we use @return in the function header instead of
> @retval.

I agree this looks like a better way of handling the failure, but I'll
leave that to you. This patch simply stops the current handling from
breaking RELEASE builds.

Regards,

Leif

> 
> Thanks for this.
> Abner
> 
> >    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));
> > --
> > 2.20.1
>