* [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: Error handling for locating firmware context
@ 2020-09-08 5:48 Abner Chang
2020-09-08 5:48 ` [edk2-platforms][PATCH 2/2] Silicon/SiFive: " Abner Chang
2020-09-11 7:59 ` [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: " Daniel Schaefer
0 siblings, 2 replies; 6+ messages in thread
From: Abner Chang @ 2020-09-08 5:48 UTC (permalink / raw)
To: devel; +Cc: abner.chang, Daniel Schaefer, Leif Lindholm
Return EFI_NOT_FOUND if opensbi firmware context can not be
located using SBI call.
Signed-off-by: Abner Chang <abner.chang@hpe.com>
Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
Cc: Leif Lindholm <leif@nuviainc.com>
---
.../ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
index 0df505d267..64c30b950d 100644
--- a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
+++ b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
@@ -851,6 +851,8 @@ SbiGetMscratchHartid (
@param[out] FirmwareContext The firmware context pointer.
@retval EFI_SUCCESS The operation succeeds.
+ @retval EFI_NOT_FOUND Failed to get the pointer of EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT.
+
**/
EFI_STATUS
EFIAPI
@@ -866,8 +868,9 @@ SbiGetFirmwareContext (
ScratchSpace = (SBI_SCRATCH *)Ret.Value;
SbiPlatform = (SBI_PLATFORM *)sbi_platform_ptr(ScratchSpace);
*FirmwareContext = (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)SbiPlatform->firmware_context;
+ } else {
+ return EFI_NOT_FOUND;
}
-
return EFI_SUCCESS;
}
--
2.25.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [edk2-platforms][PATCH 2/2] Silicon/SiFive: Error handling for locating firmware context
2020-09-08 5:48 [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: Error handling for locating firmware context Abner Chang
@ 2020-09-08 5:48 ` Abner Chang
2020-09-11 8:01 ` Daniel Schaefer
2020-09-11 7:59 ` [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: " Daniel Schaefer
1 sibling, 1 reply; 6+ messages in thread
From: Abner Chang @ 2020-09-08 5:48 UTC (permalink / raw)
To: devel; +Cc: abner.chang, Daniel Schaefer, Leif Lindholm
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) {
+ 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) {
--
2.25.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: Error handling for locating firmware context
2020-09-08 5:48 [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: Error handling for locating firmware context Abner Chang
2020-09-08 5:48 ` [edk2-platforms][PATCH 2/2] Silicon/SiFive: " Abner Chang
@ 2020-09-11 7:59 ` Daniel Schaefer
2020-09-12 6:29 ` Abner Chang
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Schaefer @ 2020-09-11 7:59 UTC (permalink / raw)
To: Abner Chang, devel; +Cc: Leif Lindholm
Oh no, what did I do here... I don't return proper error codes at all in this file.
I forgot to revise this after the initial implementation.
So, good change but we should do that for all functions in this file.
I'll do that on Monday.
On 9/8/20 1:48 PM, Abner Chang wrote:
> Return EFI_NOT_FOUND if opensbi firmware context can not be
> located using SBI call.
>
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> ---
> .../ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
> index 0df505d267..64c30b950d 100644
> --- a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
> +++ b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
> @@ -851,6 +851,8 @@ SbiGetMscratchHartid (
>
> @param[out] FirmwareContext The firmware context pointer.
> @retval EFI_SUCCESS The operation succeeds.
> + @retval EFI_NOT_FOUND Failed to get the pointer of EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT.
> +
> **/
> EFI_STATUS
> EFIAPI
> @@ -866,8 +868,9 @@ SbiGetFirmwareContext (
> ScratchSpace = (SBI_SCRATCH *)Ret.Value;
> SbiPlatform = (SBI_PLATFORM *)sbi_platform_ptr(ScratchSpace);
> *FirmwareContext = (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)SbiPlatform->firmware_context;
> + } else {
> + return EFI_NOT_FOUND;
> }
> -
> return EFI_SUCCESS;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-platforms][PATCH 2/2] Silicon/SiFive: Error handling for locating firmware context
2020-09-08 5:48 ` [edk2-platforms][PATCH 2/2] Silicon/SiFive: " Abner Chang
@ 2020-09-11 8:01 ` Daniel Schaefer
2020-09-12 6:33 ` Abner Chang
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Schaefer @ 2020-09-11 8:01 UTC (permalink / raw)
To: Abner Chang, devel; +Cc: Leif Lindholm
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) {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: Error handling for locating firmware context
2020-09-11 7:59 ` [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: " Daniel Schaefer
@ 2020-09-12 6:29 ` Abner Chang
0 siblings, 0 replies; 6+ messages in thread
From: Abner Chang @ 2020-09-12 6:29 UTC (permalink / raw)
To: Schaefer, Daniel, devel@edk2.groups.io; +Cc: Leif Lindholm
Thanks Daniel.
> -----Original Message-----
> From: Schaefer, Daniel
> Sent: Friday, September 11, 2020 3:59 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 1/2] ProcessorPkg/RiscVEdk2SbiLib:
> Error handling for locating firmware context
>
> Oh no, what did I do here... I don't return proper error codes at all in this file.
> I forgot to revise this after the initial implementation.
> So, good change but we should do that for all functions in this file.
> I'll do that on Monday.
>
> On 9/8/20 1:48 PM, Abner Chang wrote:
> > Return EFI_NOT_FOUND if opensbi firmware context can not be located
> > using SBI call.
> >
> > Signed-off-by: Abner Chang <abner.chang@hpe.com>
> > Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > ---
> > .../ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git
> > a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.
> > c
> > b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.
> > c
> > index 0df505d267..64c30b950d 100644
> > ---
> > a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.
> > c
> > +++ b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2Sbi
> > +++ Lib.c
> > @@ -851,6 +851,8 @@ SbiGetMscratchHartid (
> >
> > @param[out] FirmwareContext The firmware context pointer.
> > @retval EFI_SUCCESS The operation succeeds.
> > + @retval EFI_NOT_FOUND Failed to get the pointer of
> EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT.
> > +
> > **/
> > EFI_STATUS
> > EFIAPI
> > @@ -866,8 +868,9 @@ SbiGetFirmwareContext (
> > ScratchSpace = (SBI_SCRATCH *)Ret.Value;
> > SbiPlatform = (SBI_PLATFORM *)sbi_platform_ptr(ScratchSpace);
> > *FirmwareContext = (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT
> > *)SbiPlatform->firmware_context;
> > + } else {
> > + return EFI_NOT_FOUND;
> > }
> > -
> > return EFI_SUCCESS;
> > }
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-platforms][PATCH 2/2] Silicon/SiFive: Error handling for locating firmware context
2020-09-11 8:01 ` Daniel Schaefer
@ 2020-09-12 6:33 ` Abner Chang
0 siblings, 0 replies; 6+ messages in thread
From: Abner Chang @ 2020-09-12 6:33 UTC (permalink / raw)
To: Schaefer, Daniel, devel@edk2.groups.io; +Cc: Leif Lindholm
> -----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) {
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-12 6:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-08 5:48 [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: Error handling for locating firmware context Abner Chang
2020-09-08 5:48 ` [edk2-platforms][PATCH 2/2] Silicon/SiFive: " Abner Chang
2020-09-11 8:01 ` Daniel Schaefer
2020-09-12 6:33 ` Abner Chang
2020-09-11 7:59 ` [edk2-platforms][PATCH 1/2] ProcessorPkg/RiscVEdk2SbiLib: " Daniel Schaefer
2020-09-12 6:29 ` Abner Chang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox