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

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

[Leif Lindholm]

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 (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));
-- 
2.20.1

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

[Abner Chang]

> -----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?

> + (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.

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

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

[Leif Lindholm]

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
> 

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

[Abner Chang]

> -----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
> >

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

[Leif Lindholm]

On Sun, Aug 30, 2020 at 12:59:06 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> > > > @@ -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>

Thanks.
Pushed as 50639477fc0f.

> > 
> > 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
> > >