* [PATCH 0/1] Silicon/SiFive: Handle case of NULL FirmwareContext @ 2020-09-30 8:50 Daniel Schaefer 2020-09-30 8:50 ` [PATCH 1/1] " Daniel Schaefer 0 siblings, 1 reply; 4+ messages in thread From: Daniel Schaefer @ 2020-09-30 8:50 UTC (permalink / raw) To: devel; +Cc: Daniel Schaefer, Abner Chang, Leif Lindholm This is a replacement patch of Abner's with title ProcessorPkg/RiscVEdk2SbiLib: Error handling for locating firmware context I realized that the error handling wasn't bad but unnecessary. Those funtions can never return an error, since the SBI call they use also doesn't. Tested by booting to UEFI Shell. No new error message logged or ASSERT triggered. Cc: Abner Chang <abner.chang@hpe.com> Cc: Leif Lindholm <leif@nuviainc.com> Daniel Schaefer (1): Silicon/SiFive: Handle case of NULL FirmwareContext .../Include/Library/RiscVEdk2SbiLib.h | 12 ++--- .../PlatformPkg/Universal/Sec/SecMain.c | 11 +++-- .../Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c | 46 +++++++------------ .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 13 ++++-- 4 files changed, 36 insertions(+), 46 deletions(-) -- 2.28.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] Silicon/SiFive: Handle case of NULL FirmwareContext 2020-09-30 8:50 [PATCH 0/1] Silicon/SiFive: Handle case of NULL FirmwareContext Daniel Schaefer @ 2020-09-30 8:50 ` Daniel Schaefer 2020-09-30 13:37 ` Abner Chang [not found] ` <1639933E5766B4C0.8589@groups.io> 0 siblings, 2 replies; 4+ messages in thread From: Daniel Schaefer @ 2020-09-30 8:50 UTC (permalink / raw) To: devel; +Cc: Daniel Schaefer, Abner Chang, Leif Lindholm Abort creating the SMBIOS HOBs if there's no firmware context to get the information from. Turn SbiLib functions for getting mscratch into VOID since they can never practically fail. Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com> Cc: Abner Chang <abner.chang@hpe.com> Cc: Leif Lindholm <leif@nuviainc.com> --- .../Include/Library/RiscVEdk2SbiLib.h | 12 ++--- .../PlatformPkg/Universal/Sec/SecMain.c | 11 +++-- .../Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c | 46 +++++++------------ .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 13 ++++-- 4 files changed, 36 insertions(+), 46 deletions(-) diff --git a/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h index 558841a970ce..f81ea06b05b0 100644 --- a/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h +++ b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h @@ -514,9 +514,8 @@ SbiVendorCall ( access the firmware context. @param[out] ScratchSpace The scratch space pointer. - @retval EFI_SUCCESS The operation succeeds. **/ -EFI_STATUS +VOID EFIAPI SbiGetMscratch ( OUT SBI_SCRATCH **ScratchSpace @@ -527,9 +526,8 @@ SbiGetMscratch ( @param[in] HartId The hart id. @param[out] ScratchSpace The scratch space pointer. - @retval EFI_SUCCESS The operation succeeds. **/ -EFI_STATUS +VOID EFIAPI SbiGetMscratchHartid ( IN UINTN HartId, @@ -540,9 +538,8 @@ SbiGetMscratchHartid ( Get firmware context of the calling hart. @param[out] FirmwareContext The firmware context pointer. - @retval EFI_SUCCESS The operation succeeds. **/ -EFI_STATUS +VOID EFIAPI SbiGetFirmwareContext ( OUT EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT **FirmwareContext @@ -552,9 +549,8 @@ SbiGetFirmwareContext ( Set firmware context of the calling hart. @param[in] FirmwareContext The firmware context pointer. - @retval EFI_SUCCESS The operation succeeds. **/ -EFI_STATUS +VOID EFIAPI SbiSetFirmwareContext ( IN EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *FirmwareContext diff --git a/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c b/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c index 877777bfa1ab..fa9ecd789a57 100644 --- a/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c +++ b/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c @@ -415,7 +415,10 @@ EFI_STATUS EFIAPI TemporaryRamDone ( return EFI_SUCCESS; } -/** Handles SBI calls of EDK2's SBI FW extension +/** + Handles SBI calls of EDK2's SBI FW extension. + + The return value is the error code returned by the SBI call. @param[in] ExtId The extension ID of the FW extension. @param[in] FuncId The called function ID. @@ -424,7 +427,7 @@ EFI_STATUS EFIAPI TemporaryRamDone ( @param[out] OutTrap Trap info for trapping further, see OpenSBI code. Is ignored if return value is not SBI_ETRAP. - @retval 0 If the handler succeeds. + @retval SBI_OK If the handler succeeds. @retval SBI_ENOTSUPP If there's no function with the given ID. @retval SBI_ETRAP If the called SBI functions wants to trap further. **/ @@ -436,7 +439,7 @@ STATIC int SbiEcallFirmwareHandler ( OUT struct sbi_trap_info *OutTrap ) { - int Ret = 0; + int Ret = SBI_OK; switch (FuncId) { case SBI_EXT_FW_MSCRATCH_FUNC: @@ -447,6 +450,8 @@ STATIC int SbiEcallFirmwareHandler ( break; default: Ret = SBI_ENOTSUPP; + DEBUG ((DEBUG_ERROR, "%a: Called SBI firmware ecall with invalid function ID\n", __FUNCTION__)); + ASSERT (FALSE); }; return Ret; diff --git a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c index 0df505d2675b..9bbeaaec3f7a 100644 --- a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c +++ b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c @@ -801,9 +801,8 @@ SbiVendorCall ( access the firmware context. @param[out] ScratchSpace The scratch space pointer. - @retval EFI_SUCCESS The operation succeeds. **/ -EFI_STATUS +VOID EFIAPI SbiGetMscratch ( OUT SBI_SCRATCH **ScratchSpace @@ -811,11 +810,10 @@ SbiGetMscratch ( { SbiRet Ret = SbiCall (SBI_EDK2_FW_EXT, SBI_EXT_FW_MSCRATCH_FUNC, 0); - if (!Ret.Error) { - *ScratchSpace = (SBI_SCRATCH *)Ret.Value; - } + // Our ecall handler never returns an error, only when the func id is invalid + ASSERT (Ret.Error == SBI_OK); - return EFI_SUCCESS; + *ScratchSpace = (SBI_SCRATCH *)Ret.Value; } /** @@ -823,9 +821,8 @@ SbiGetMscratch ( @param[in] HartId The hart id. @param[out] ScratchSpace The scratch space pointer. - @retval EFI_SUCCESS The operation succeeds. **/ -EFI_STATUS +VOID EFIAPI SbiGetMscratchHartid ( IN UINTN HartId, @@ -839,11 +836,10 @@ SbiGetMscratchHartid ( HartId ); - if (!Ret.Error) { - *ScratchSpace = (SBI_SCRATCH *)Ret.Value; - } + // Our ecall handler never returns an error, only when the func id is invalid + ASSERT (Ret.Error == SBI_OK); - return EFI_SUCCESS; + *ScratchSpace = (SBI_SCRATCH *)Ret.Value; } /** @@ -852,7 +848,7 @@ SbiGetMscratchHartid ( @param[out] FirmwareContext The firmware context pointer. @retval EFI_SUCCESS The operation succeeds. **/ -EFI_STATUS +VOID EFIAPI SbiGetFirmwareContext ( OUT EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT **FirmwareContext @@ -860,24 +856,18 @@ SbiGetFirmwareContext ( { SBI_SCRATCH *ScratchSpace; SBI_PLATFORM *SbiPlatform; - SbiRet Ret = SbiCall (SBI_EDK2_FW_EXT, SBI_EXT_FW_MSCRATCH_FUNC, 0); - if (!Ret.Error) { - ScratchSpace = (SBI_SCRATCH *)Ret.Value; - SbiPlatform = (SBI_PLATFORM *)sbi_platform_ptr(ScratchSpace); - *FirmwareContext = (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)SbiPlatform->firmware_context; - } - - return EFI_SUCCESS; + SbiGetMscratch(&ScratchSpace); + SbiPlatform = (SBI_PLATFORM *)sbi_platform_ptr(ScratchSpace); + *FirmwareContext = (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)SbiPlatform->firmware_context; } /** Set firmware context of the calling hart. @param[in] FirmwareContext The firmware context pointer. - @retval EFI_SUCCESS The operation succeeds. **/ -EFI_STATUS +VOID EFIAPI SbiSetFirmwareContext ( IN EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *FirmwareContext @@ -885,13 +875,9 @@ SbiSetFirmwareContext ( { SBI_SCRATCH *ScratchSpace; SBI_PLATFORM *SbiPlatform; - SbiRet Ret = SbiCall (SBI_EDK2_FW_EXT, SBI_EXT_FW_MSCRATCH_FUNC, 0); - if (!Ret.Error) { - ScratchSpace = (SBI_SCRATCH *)Ret.Value; - SbiPlatform = (SBI_PLATFORM *)sbi_platform_ptr (ScratchSpace); - SbiPlatform->firmware_context = (UINTN)FirmwareContext; - } + SbiGetMscratch(&ScratchSpace); - return EFI_SUCCESS; + SbiPlatform = (SBI_PLATFORM *)sbi_platform_ptr (ScratchSpace); + SbiPlatform->firmware_context = (UINTN)FirmwareContext; } diff --git a/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c b/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c index edeabf028ff8..88f36cbbe299 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 @@ -56,7 +56,6 @@ 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__)); @@ -64,9 +63,14 @@ CreateU54E51CoreProcessorSpecificDataHob ( return EFI_INVALID_PARAMETER; } - Status = SbiGetFirmwareContext (&FirmwareContext); - ASSERT_EFI_ERROR (Status); + SbiGetFirmwareContext (&FirmwareContext); + ASSERT (FirmwareContext != NULL); + if (FirmwareContext == NULL) { + DEBUG ((DEBUG_ERROR, "Failed to get the pointer of EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT of hart %d\n", HartId)); + return EFI_NOT_FOUND; + } 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) { @@ -102,7 +106,6 @@ CreateU54E51CoreProcessorSpecificDataHob ( ProcessorSpecDataHob.ProcessorSpecificData.SupervisorModeXlen = RegisterLen64; } - DebugPrintHartSpecificInfo (&ProcessorSpecDataHob); // -- 2.28.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] Silicon/SiFive: Handle case of NULL FirmwareContext 2020-09-30 8:50 ` [PATCH 1/1] " Daniel Schaefer @ 2020-09-30 13:37 ` Abner Chang [not found] ` <1639933E5766B4C0.8589@groups.io> 1 sibling, 0 replies; 4+ messages in thread From: Abner Chang @ 2020-09-30 13:37 UTC (permalink / raw) To: Schaefer, Daniel, devel@edk2.groups.io; +Cc: Leif Lindholm Reviewed-by: Abner Chang <abner.chang@hpe.com> > -----Original Message----- > From: Schaefer, Daniel > Sent: Wednesday, September 30, 2020 4:50 PM > To: devel@edk2.groups.io > Cc: Schaefer, Daniel <daniel.schaefer@hpe.com>; Chang, Abner (HPS > SW/FW Technologist) <abner.chang@hpe.com>; Leif Lindholm > <leif@nuviainc.com> > Subject: [PATCH 1/1] Silicon/SiFive: Handle case of NULL FirmwareContext > > Abort creating the SMBIOS HOBs if there's no firmware context to get the > information from. > Turn SbiLib functions for getting mscratch into VOID since they can never > practically fail. > > Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com> > Cc: Abner Chang <abner.chang@hpe.com> > Cc: Leif Lindholm <leif@nuviainc.com> > --- > .../Include/Library/RiscVEdk2SbiLib.h | 12 ++--- > .../PlatformPkg/Universal/Sec/SecMain.c | 11 +++-- > .../Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c | 46 +++++++------------ > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 13 ++++-- > 4 files changed, 36 insertions(+), 46 deletions(-) > > diff --git a/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > index 558841a970ce..f81ea06b05b0 100644 > --- a/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > +++ b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > @@ -514,9 +514,8 @@ SbiVendorCall ( > access the firmware context. @param[out] ScratchSpace The scratch > space pointer.- @retval EFI_SUCCESS The operation succeeds. **/- > EFI_STATUS+VOID EFIAPI SbiGetMscratch ( OUT SBI_SCRATCH > **ScratchSpace@@ -527,9 +526,8 @@ SbiGetMscratch ( > @param[in] HartId The hart id. @param[out] ScratchSpace The > scratch space pointer.- @retval EFI_SUCCESS The operation succeeds. > **/-EFI_STATUS+VOID EFIAPI SbiGetMscratchHartid ( IN UINTN > HartId,@@ -540,9 +538,8 @@ SbiGetMscratchHartid ( > Get firmware context of the calling hart. @param[out] FirmwareContext > The firmware context pointer.- @retval EFI_SUCCESS The operation > succeeds. **/-EFI_STATUS+VOID EFIAPI SbiGetFirmwareContext ( OUT > EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT **FirmwareContext@@ -552,9 > +549,8 @@ SbiGetFirmwareContext ( > Set firmware context of the calling hart. @param[in] FirmwareContext > The firmware context pointer.- @retval EFI_SUCCESS The operation > succeeds. **/-EFI_STATUS+VOID EFIAPI SbiSetFirmwareContext ( IN > EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *FirmwareContextdiff --git > a/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c b/Platform/RISC- > V/PlatformPkg/Universal/Sec/SecMain.c > index 877777bfa1ab..fa9ecd789a57 100644 > --- a/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c > +++ b/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c > @@ -415,7 +415,10 @@ EFI_STATUS EFIAPI TemporaryRamDone ( > return EFI_SUCCESS; } -/** Handles SBI calls of EDK2's SBI FW > extension+/**+ Handles SBI calls of EDK2's SBI FW extension.++ The return > value is the error code returned by the SBI call. @param[in] ExtId The > extension ID of the FW extension. @param[in] FuncId The called > function ID.@@ -424,7 +427,7 @@ EFI_STATUS EFIAPI TemporaryRamDone ( > @param[out] OutTrap Trap info for trapping further, see OpenSBI code. > Is ignored if return value is not SBI_ETRAP. - @retval 0 If the handler > succeeds.+ @retval SBI_OK If the handler succeeds. @retval > SBI_ENOTSUPP If there's no function with the given ID. @retval > SBI_ETRAP If the called SBI functions wants to trap further. **/@@ -436,7 > +439,7 @@ STATIC int SbiEcallFirmwareHandler ( > OUT struct sbi_trap_info *OutTrap ) {- int Ret = 0;+ int Ret = SBI_OK; > switch (FuncId) { case SBI_EXT_FW_MSCRATCH_FUNC:@@ -447,6 +450,8 > @@ STATIC int SbiEcallFirmwareHandler ( > break; default: Ret = SBI_ENOTSUPP;+ DEBUG ((DEBUG_ERROR, > "%a: Called SBI firmware ecall with invalid function ID\n", __FUNCTION__));+ > ASSERT (FALSE); }; return Ret;diff --git a/Silicon/RISC- > V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c b/Silicon/RISC- > V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c > index 0df505d2675b..9bbeaaec3f7a 100644 > --- a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c > +++ b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLi > +++ b.c > @@ -801,9 +801,8 @@ SbiVendorCall ( > access the firmware context. @param[out] ScratchSpace The scratch > space pointer.- @retval EFI_SUCCESS The operation succeeds. **/- > EFI_STATUS+VOID EFIAPI SbiGetMscratch ( OUT SBI_SCRATCH > **ScratchSpace@@ -811,11 +810,10 @@ SbiGetMscratch ( > { SbiRet Ret = SbiCall (SBI_EDK2_FW_EXT, SBI_EXT_FW_MSCRATCH_FUNC, > 0); - if (!Ret.Error) {- *ScratchSpace = (SBI_SCRATCH *)Ret.Value;- }+ // > Our ecall handler never returns an error, only when the func id is invalid+ > ASSERT (Ret.Error == SBI_OK); - return EFI_SUCCESS;+ *ScratchSpace = > (SBI_SCRATCH *)Ret.Value; } /**@@ -823,9 +821,8 @@ SbiGetMscratch ( > @param[in] HartId The hart id. @param[out] ScratchSpace The > scratch space pointer.- @retval EFI_SUCCESS The operation succeeds. > **/-EFI_STATUS+VOID EFIAPI SbiGetMscratchHartid ( IN UINTN > HartId,@@ -839,11 +836,10 @@ SbiGetMscratchHartid ( > HartId ); - if (!Ret.Error) {- *ScratchSpace = (SBI_SCRATCH > *)Ret.Value;- }+ // Our ecall handler never returns an error, only when the > func id is invalid+ ASSERT (Ret.Error == SBI_OK); - return EFI_SUCCESS;+ > *ScratchSpace = (SBI_SCRATCH *)Ret.Value; } /**@@ -852,7 +848,7 @@ > SbiGetMscratchHartid ( > @param[out] FirmwareContext The firmware context pointer. @retval > EFI_SUCCESS The operation succeeds. **/-EFI_STATUS+VOID EFIAPI > SbiGetFirmwareContext ( OUT EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT > **FirmwareContext@@ -860,24 +856,18 @@ SbiGetFirmwareContext ( > { SBI_SCRATCH *ScratchSpace; SBI_PLATFORM *SbiPlatform;- SbiRet Ret > = SbiCall (SBI_EDK2_FW_EXT, SBI_EXT_FW_MSCRATCH_FUNC, 0); - if > (!Ret.Error) {- ScratchSpace = (SBI_SCRATCH *)Ret.Value;- SbiPlatform = > (SBI_PLATFORM *)sbi_platform_ptr(ScratchSpace);- *FirmwareContext = > (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)SbiPlatform- > >firmware_context;- }-- return EFI_SUCCESS;+ > SbiGetMscratch(&ScratchSpace);+ SbiPlatform = (SBI_PLATFORM > *)sbi_platform_ptr(ScratchSpace);+ *FirmwareContext = > (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)SbiPlatform- > >firmware_context; } /** Set firmware context of the calling hart. > @param[in] FirmwareContext The firmware context pointer.- @retval > EFI_SUCCESS The operation succeeds. **/-EFI_STATUS+VOID EFIAPI > SbiSetFirmwareContext ( IN EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT > *FirmwareContext@@ -885,13 +875,9 @@ SbiSetFirmwareContext ( > { SBI_SCRATCH *ScratchSpace; SBI_PLATFORM *SbiPlatform;- SbiRet Ret > = SbiCall (SBI_EDK2_FW_EXT, SBI_EXT_FW_MSCRATCH_FUNC, 0); - if > (!Ret.Error) {- ScratchSpace = (SBI_SCRATCH *)Ret.Value;- SbiPlatform = > (SBI_PLATFORM *)sbi_platform_ptr (ScratchSpace);- SbiPlatform- > >firmware_context = (UINTN)FirmwareContext;- }+ > SbiGetMscratch(&ScratchSpace); - return EFI_SUCCESS;+ SbiPlatform = > (SBI_PLATFORM *)sbi_platform_ptr (ScratchSpace);+ SbiPlatform- > >firmware_context = (UINTN)FirmwareContext; }diff --git > a/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c > b/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c > index edeabf028ff8..88f36cbbe299 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@@ -56,7 > +56,6 @@ 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__)); @@ -64,9 +63,14 @@ > CreateU54E51CoreProcessorSpecificDataHob ( > return EFI_INVALID_PARAMETER; } - Status = SbiGetFirmwareContext > (&FirmwareContext);- ASSERT_EFI_ERROR (Status);+ > SbiGetFirmwareContext (&FirmwareContext);+ ASSERT > (FirmwareContext != NULL);+ if (FirmwareContext == NULL) {+ DEBUG > ((DEBUG_ERROR, "Failed to get the pointer of > EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT of hart %d\n", HartId));+ return > EFI_NOT_FOUND;+ } 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) {@@ -102,7 +106,6 @@ > CreateU54E51CoreProcessorSpecificDataHob ( > ProcessorSpecDataHob.ProcessorSpecificData.SupervisorModeXlen = > RegisterLen64; } - DebugPrintHartSpecificInfo (&ProcessorSpecDataHob); > //-- > 2.28.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <1639933E5766B4C0.8589@groups.io>]
* Re: [edk2-devel] [PATCH 1/1] Silicon/SiFive: Handle case of NULL FirmwareContext [not found] ` <1639933E5766B4C0.8589@groups.io> @ 2020-10-02 2:40 ` Abner Chang 0 siblings, 0 replies; 4+ messages in thread From: Abner Chang @ 2020-10-02 2:40 UTC (permalink / raw) To: devel@edk2.groups.io, Chang, Abner (HPS SW/FW Technologist), Schaefer, Daniel Cc: Leif Lindholm Merged to edk2-platforms. > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Abner Chang > Sent: Wednesday, September 30, 2020 9:37 PM > To: Schaefer, Daniel <daniel.schaefer@hpe.com>; devel@edk2.groups.io > Cc: Leif Lindholm <leif@nuviainc.com> > Subject: Re: [edk2-devel] [PATCH 1/1] Silicon/SiFive: Handle case of NULL > FirmwareContext > > Reviewed-by: Abner Chang <abner.chang@hpe.com> > > > -----Original Message----- > > From: Schaefer, Daniel > > Sent: Wednesday, September 30, 2020 4:50 PM > > To: devel@edk2.groups.io > > Cc: Schaefer, Daniel <daniel.schaefer@hpe.com>; Chang, Abner (HPS > > SW/FW Technologist) <abner.chang@hpe.com>; Leif Lindholm > > <leif@nuviainc.com> > > Subject: [PATCH 1/1] Silicon/SiFive: Handle case of NULL > > FirmwareContext > > > > Abort creating the SMBIOS HOBs if there's no firmware context to get > > the information from. > > Turn SbiLib functions for getting mscratch into VOID since they can > > never practically fail. > > > > Signed-off-by: Daniel Schaefer <daniel.schaefer@hpe.com> > > Cc: Abner Chang <abner.chang@hpe.com> > > Cc: Leif Lindholm <leif@nuviainc.com> > > --- > > .../Include/Library/RiscVEdk2SbiLib.h | 12 ++--- > > .../PlatformPkg/Universal/Sec/SecMain.c | 11 +++-- > > .../Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c | 46 +++++++------------ > > .../Library/PeiCoreInfoHobLib/CoreInfoHob.c | 13 ++++-- > > 4 files changed, 36 insertions(+), 46 deletions(-) > > > > diff --git > > a/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > > b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > > index 558841a970ce..f81ea06b05b0 100644 > > --- a/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > > +++ b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > > @@ -514,9 +514,8 @@ SbiVendorCall ( > > access the firmware context. @param[out] ScratchSpace The scratch > > space pointer.- @retval EFI_SUCCESS The operation succeeds. **/- > > EFI_STATUS+VOID EFIAPI SbiGetMscratch ( OUT SBI_SCRATCH > > **ScratchSpace@@ -527,9 +526,8 @@ SbiGetMscratch ( > > @param[in] HartId The hart id. @param[out] ScratchSpace > The > > scratch space pointer.- @retval EFI_SUCCESS The operation > succeeds. > > **/-EFI_STATUS+VOID EFIAPI SbiGetMscratchHartid ( IN UINTN > > HartId,@@ -540,9 +538,8 @@ SbiGetMscratchHartid ( > > Get firmware context of the calling hart. @param[out] FirmwareContext > > The firmware context pointer.- @retval EFI_SUCCESS The operation > > succeeds. **/-EFI_STATUS+VOID EFIAPI SbiGetFirmwareContext ( OUT > > EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT **FirmwareContext@@ -552,9 > > +549,8 @@ SbiGetFirmwareContext ( > > Set firmware context of the calling hart. @param[in] FirmwareContext > > The firmware context pointer.- @retval EFI_SUCCESS The operation > > succeeds. **/-EFI_STATUS+VOID EFIAPI SbiSetFirmwareContext ( IN > > EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *FirmwareContextdiff --git > > a/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c > b/Platform/RISC- > > V/PlatformPkg/Universal/Sec/SecMain.c > > index 877777bfa1ab..fa9ecd789a57 100644 > > --- a/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c > > +++ b/Platform/RISC-V/PlatformPkg/Universal/Sec/SecMain.c > > @@ -415,7 +415,10 @@ EFI_STATUS EFIAPI TemporaryRamDone ( > > return EFI_SUCCESS; } -/** Handles SBI calls of EDK2's SBI FW > > extension+/**+ Handles SBI calls of EDK2's SBI FW extension.++ The > > extension+return > > value is the error code returned by the SBI call. @param[in] ExtId The > > extension ID of the FW extension. @param[in] FuncId The called > > function ID.@@ -424,7 +427,7 @@ EFI_STATUS EFIAPI TemporaryRamDone > ( > > @param[out] OutTrap Trap info for trapping further, see OpenSBI code. > > Is ignored if return value is not SBI_ETRAP. - @retval 0 If the > handler > > succeeds.+ @retval SBI_OK If the handler succeeds. @retval > > SBI_ENOTSUPP If there's no function with the given ID. @retval > > SBI_ETRAP If the called SBI functions wants to trap further. **/@@ - > 436,7 > > +439,7 @@ STATIC int SbiEcallFirmwareHandler ( > > OUT struct sbi_trap_info *OutTrap ) {- int Ret = 0;+ int Ret = SBI_OK; > > switch (FuncId) { case SBI_EXT_FW_MSCRATCH_FUNC:@@ -447,6 +450,8 > > @@ STATIC int SbiEcallFirmwareHandler ( > > break; default: Ret = SBI_ENOTSUPP;+ DEBUG ((DEBUG_ERROR, > > "%a: Called SBI firmware ecall with invalid function ID\n", > __FUNCTION__));+ > > ASSERT (FALSE); }; return Ret;diff --git a/Silicon/RISC- > > V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c > > b/Silicon/RISC- > > V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c > > index 0df505d2675b..9bbeaaec3f7a 100644 > > --- > > a/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib. > > c > > +++ b/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2Sbi > > +++ Li > > +++ b.c > > @@ -801,9 +801,8 @@ SbiVendorCall ( > > access the firmware context. @param[out] ScratchSpace The scratch > > space pointer.- @retval EFI_SUCCESS The operation succeeds. **/- > > EFI_STATUS+VOID EFIAPI SbiGetMscratch ( OUT SBI_SCRATCH > > **ScratchSpace@@ -811,11 +810,10 @@ SbiGetMscratch ( > > { SbiRet Ret = SbiCall (SBI_EDK2_FW_EXT, > SBI_EXT_FW_MSCRATCH_FUNC, > > 0); - if (!Ret.Error) {- *ScratchSpace = (SBI_SCRATCH *)Ret.Value;- }+ // > > Our ecall handler never returns an error, only when the func id is > > invalid+ ASSERT (Ret.Error == SBI_OK); - return EFI_SUCCESS;+ > > *ScratchSpace = (SBI_SCRATCH *)Ret.Value; } /**@@ -823,9 +821,8 @@ > SbiGetMscratch ( > > @param[in] HartId The hart id. @param[out] ScratchSpace > The > > scratch space pointer.- @retval EFI_SUCCESS The operation > succeeds. > > **/-EFI_STATUS+VOID EFIAPI SbiGetMscratchHartid ( IN UINTN > > HartId,@@ -839,11 +836,10 @@ SbiGetMscratchHartid ( > > HartId ); - if (!Ret.Error) {- *ScratchSpace = > (SBI_SCRATCH > > *)Ret.Value;- }+ // Our ecall handler never returns an error, only > > when the func id is invalid+ ASSERT (Ret.Error == SBI_OK); - return > > EFI_SUCCESS;+ *ScratchSpace = (SBI_SCRATCH *)Ret.Value; } /**@@ > > -852,7 +848,7 @@ SbiGetMscratchHartid ( > > @param[out] FirmwareContext The firmware context pointer. > @retval > > EFI_SUCCESS The operation succeeds. **/-EFI_STATUS+VOID EFIAPI > > SbiGetFirmwareContext ( OUT > EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT > > **FirmwareContext@@ -860,24 +856,18 @@ SbiGetFirmwareContext ( > > { SBI_SCRATCH *ScratchSpace; SBI_PLATFORM *SbiPlatform;- SbiRet > Ret > > = SbiCall (SBI_EDK2_FW_EXT, SBI_EXT_FW_MSCRATCH_FUNC, 0); - if > > (!Ret.Error) {- ScratchSpace = (SBI_SCRATCH *)Ret.Value;- SbiPlatform = > > (SBI_PLATFORM *)sbi_platform_ptr(ScratchSpace);- *FirmwareContext = > > (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)SbiPlatform- > > >firmware_context;- }-- return EFI_SUCCESS;+ > > SbiGetMscratch(&ScratchSpace);+ SbiPlatform = (SBI_PLATFORM > > *)sbi_platform_ptr(ScratchSpace);+ *FirmwareContext = > > (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)SbiPlatform- > > >firmware_context; } /** Set firmware context of the calling hart. > > @param[in] FirmwareContext The firmware context pointer.- @retval > > EFI_SUCCESS The operation succeeds. **/-EFI_STATUS+VOID EFIAPI > > SbiSetFirmwareContext ( IN EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT > > *FirmwareContext@@ -885,13 +875,9 @@ SbiSetFirmwareContext ( > > { SBI_SCRATCH *ScratchSpace; SBI_PLATFORM *SbiPlatform;- SbiRet > Ret > > = SbiCall (SBI_EDK2_FW_EXT, SBI_EXT_FW_MSCRATCH_FUNC, 0); - if > > (!Ret.Error) {- ScratchSpace = (SBI_SCRATCH *)Ret.Value;- SbiPlatform = > > (SBI_PLATFORM *)sbi_platform_ptr (ScratchSpace);- SbiPlatform- > > >firmware_context = (UINTN)FirmwareContext;- }+ > > SbiGetMscratch(&ScratchSpace); - return EFI_SUCCESS;+ SbiPlatform = > > (SBI_PLATFORM *)sbi_platform_ptr (ScratchSpace);+ SbiPlatform- > > >firmware_context = (UINTN)FirmwareContext; }diff --git > > a/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c > > b/Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c > > index edeabf028ff8..88f36cbbe299 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@@ > > -56,7 > > +56,6 @@ 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__)); @@ -64,9 +63,14 @@ > > CreateU54E51CoreProcessorSpecificDataHob ( > > return EFI_INVALID_PARAMETER; } - Status = SbiGetFirmwareContext > > (&FirmwareContext);- ASSERT_EFI_ERROR (Status);+ > > SbiGetFirmwareContext (&FirmwareContext);+ ASSERT > > (FirmwareContext != NULL);+ if (FirmwareContext == NULL) {+ DEBUG > > ((DEBUG_ERROR, "Failed to get the pointer of > > EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT of hart %d\n", HartId));+ > return > > EFI_NOT_FOUND;+ } 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) {@@ -102,7 +106,6 @@ > CreateU54E51CoreProcessorSpecificDataHob ( > > ProcessorSpecDataHob.ProcessorSpecificData.SupervisorModeXlen = > > RegisterLen64; } - DebugPrintHartSpecificInfo (&ProcessorSpecDataHob); > > //-- > > 2.28.0 > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-02 2:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-30 8:50 [PATCH 0/1] Silicon/SiFive: Handle case of NULL FirmwareContext Daniel Schaefer
2020-09-30 8:50 ` [PATCH 1/1] " Daniel Schaefer
2020-09-30 13:37 ` Abner Chang
[not found] ` <1639933E5766B4C0.8589@groups.io>
2020-10-02 2:40 ` [edk2-devel] " Abner Chang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox