From: "Abner Chang" <abner.chang@hpe.com>
To: "Schaefer, Daniel" <daniel.schaefer@hpe.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: Leif Lindholm <leif@nuviainc.com>
Subject: Re: [PATCH 1/1] Silicon/SiFive: Handle case of NULL FirmwareContext
Date: Wed, 30 Sep 2020 13:37:14 +0000 [thread overview]
Message-ID: <CS1PR8401MB1144542D8EA738685A7FE917FF330@CS1PR8401MB1144.NAMPRD84.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20200930085005.27148-2-daniel.schaefer@hpe.com>
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
next prev parent reply other threads:[~2020-09-30 13:37 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
[not found] ` <1639933E5766B4C0.8589@groups.io>
2020-10-02 2:40 ` [edk2-devel] " Abner Chang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CS1PR8401MB1144542D8EA738685A7FE917FF330@CS1PR8401MB1144.NAMPRD84.PROD.OUTLOOK.COM \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox