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

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

> -----Original Message-----
> From: Leif Lindholm [mailto:leif@nuviainc.com]
> Sent: Sunday, August 30, 2020 7:34 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> Cc: devel@edk2.groups.io; Chen, Gilbert <gilbert.chen@hpe.com>; Schaefer,
> Daniel <daniel.schaefer@hpe.com>
> Subject: Re: [PATCH edk2-platforms 1/1] Silicon/SiFive: fix RELEASE builds of
> U54 PeiCoreInfoHobLib
> 
> 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?
> 
> INVALID URI REMOVED
> 3A__edk2.groups.io_g_devel_message_64794&d=DwIBAg&c=C5b8zRQO1mi
> GmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=
> UkdQYKg5exPcuGmzwjtBQvaRGX5SPNiaP9ko8nTDWgQ&s=r0p7ssf1Q8Y2070
> L71hNFsILUzkHznlm5k8mpyVeV9k&e=
> It's your editor.
Ok.
> 
> > > + (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.
Sure!

Reviewed-by: Abner Chang <abner.chang@hpe.com>
> 
> 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
> >