> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Thursday, October 3, 2019 3:43 AM
> To: devel@edk2.groups.io; Chen, Gilbert <gilbert.c...@hpe.com>
> Cc: Palmer Dabbelt <pal...@sifive.com>
> Subject: Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 06/14]
> RiscV/Universal: Initial version of common RISC-V SEC module
>
> On Thu, Sep 19, 2019 at 11:51:23AM +0800, Gilbert Chen wrote:
> > Common RISC-V SEC module for RISC-V platforms.
>
> If this is common to RISC-V platforms, this really should live in
> edk2/RiscVPkg.
Edk2/RiscVPlatformPkg is the package of SEC module now. You will see this in v3
edk2-staging patches.
>
> > Signed-off-by: Gilbert Chen <gilbert.c...@hpe.com>
> > ---
> > Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S | 438
> ++++++++++++++++++++
> > Platform/RiscV/Universal/Sec/SecMain.c | 524
> ++++++++++++++++++++++++
> > Platform/RiscV/Universal/Sec/SecMain.h | 57 +++
> > Platform/RiscV/Universal/Sec/SecMain.inf | 75 ++++
> > 4 files changed, 1094 insertions(+)
> > create mode 100644 Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S
> > create mode 100644 Platform/RiscV/Universal/Sec/SecMain.c
> > create mode 100644 Platform/RiscV/Universal/Sec/SecMain.h
> > create mode 100644 Platform/RiscV/Universal/Sec/SecMain.inf
> >
> > diff --git a/Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S
> > b/Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S
> > new file mode 100644
> > index 00000000..18b54b84
> > --- /dev/null
> > +++ b/Platform/RiscV/Universal/Sec/Riscv64/SecEntry.S
> > @@ -0,0 +1,438 @@
> > +/*
> > + * Copyright (c) 2019 , Hewlett Packard Enterprise Development LP. All
> rights reserved.
> > + *
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> > + *
> > + * Authors:
> > + * Anup Patel <anup.pa...@wdc.com>
>
> If Anup is the author, he should be indicated in the commit as the Author.
>
> If you further modify the patch, you can add a comment above your
> signed-off-by: of the form
> [Updated coding style issues.]
> Signed-off-by: ...
>
> > + *
> > + */
> > +
> > +#include <Base.h>
> > +#include <RiscV.h>
> > +#include <sbi/riscv_asm.h>
> > +#include <sbi/riscv_encoding.h>
> > +#include <sbi/sbi_platform.h>
> > +#include <sbi/sbi_scratch.h>
> > +#include <sbi/sbi_trap.h>
> > +
> > +#include <SecMain.h>
> > +#include <sbi/SbiFirmwareContext.h>
> > +
> > +.text
> > +.align 3
> > +.global ASM_PFX(_ModuleEntryPoint)
> > +ASM_PFX(_ModuleEntryPoint):
> > + /*
> > + * Jump to warm-boot if this is not the selected core booting,
> > + */
> > + csrr a6, CSR_MHARTID
> > + li a5, FixedPcdGet32 (PcdBootHartId)
> > + bne a6, a5, _wait_for_boot_hart
>
> We don't actually have a coding style for assembler, but I find it really
> helps
> readability to have the arguments aligned across all lines. Compare (for
> example) with OvmfPkg/Sec/X64/SecEntry.nasm.
>
> > +
> > + // light LED on
> > + li a5, 0x54002000
> > + li a4, 0xff
> > + sw a4, 0x08(a5)
> > + li a4, 0x11
> > + sw a4, 0x0c(a5)
> > +
> > + li ra, 0
> > + call _reset_regs
> > +
> > + /* Preload HART details
> > + * s7 -> HART Count
> > + * s8 -> HART Stack Size
> > + */
> > + li s7, FixedPcdGet32 (PcdHartCount) li s8, FixedPcdGet32
> > + (PcdOpenSbiStackSize)
> > +
> > + /* Setup scratch space for all the HARTs*/
> > + li tp, FixedPcdGet32 (PcdScratchRamBase)
> > + mul a5, s7, s8
> > + add tp, tp, a5
> > + /* Keep a copy of tp */
> > + add t3, tp, zero
> > + /* Counter */
> > + li t2, 1
> > + /* hartid 0 is mandated by ISA */
> > + li t1, 0
> > +_scratch_init:
> > + add tp, t3, zero
> > + mul a5, s8, t1
> > + sub tp, tp, a5
> > + li a5, SBI_SCRATCH_SIZE
> > + sub tp, tp, a5
> > +
> > + /* Initialize scratch space */
> > + li a4, FixedPcdGet32 (PcdFwStartAddress) li a5, FixedPcdGet32
> > + (PcdFwEndAddress) sub a5, a5, a4 sd a4,
> > + SBI_SCRATCH_FW_START_OFFSET(tp) sd a5,
> > + SBI_SCRATCH_FW_SIZE_OFFSET(tp)
>
> Can you add a blank line here?
>
> > + /* Note: fw_next_arg1() uses a0, a1, and ra */ call fw_next_arg1
> > + sd a0, SBI_SCRATCH_NEXT_ARG1_OFFSET(tp)
>
> Can you add a blank line here?
>
> > + /* Note: fw_next_addr() uses a0, a1, and ra */ call fw_next_addr
>
> Can you add a blank line here?
>
> > + sd a0, SBI_SCRATCH_NEXT_ADDR_OFFSET(tp) li a4, PRV_S sd a4,
> > + SBI_SCRATCH_NEXT_MODE_OFFSET(tp) la a4, _start_warm sd a4,
> > + SBI_SCRATCH_WARMBOOT_ADDR_OFFSET(tp)
> > + la a4, platform
> > + sd a4, SBI_SCRATCH_PLATFORM_ADDR_OFFSET(tp)
> > + la a4, _hartid_to_scratch
> > + sd a4, SBI_SCRATCH_HARTID_TO_SCRATCH_OFFSET(tp)
> > + sd zero, SBI_SCRATCH_TMP0_OFFSET(tp)
>
> Can you add a blank line here?
>
> > +#ifdef FW_OPTIONS
> > + li a4, FW_OPTIONS
> > + sd a4, SBI_SCRATCH_OPTIONS_OFFSET(tp) #else
> > + sd zero, SBI_SCRATCH_OPTIONS_OFFSET(tp) #endif
>
> Can you add a blank line here?
>
> > + add t1, t1, t2
> > + blt t1, s7, _scratch_init
> > +
> > + /* Fill-out temporary memory with 55aa*/
> > + li a4, FixedPcdGet32 (PcdTemporaryRamBase)
> > + li a5, FixedPcdGet32 (PcdTemporaryRamSize)
> > + add a5, a4, a5
> > +1:
> > + li a3, 0x5AA55AA55AA55AA5
> > + sd a3, (a4)
> > + add a4, a4, __SIZEOF_POINTER__
> > + blt a4, a5, 1b
> > +
> > + /* Update boot hart flag */
> > + la a4, _boot_hart_done
> > + li a5, 1
> > + sd a5, (a4)
> > +
> > + /* Wait for boot hart */
> > +_wait_for_boot_hart:
> > + la a4, _boot_hart_done
> > + ld a5, (a4)
> > + /* Reduce the bus traffic so that boot hart may proceed faster */
> > + nop
> > + nop
> > + nop
> > + beqz a5, _wait_for_boot_hart
> > +
> > +_start_warm:
> > + li ra, 0
> > + call _reset_regs
> > +
> > + /* Disable and clear all interrupts */ csrw CSR_MIE, zero csrw
> > + CSR_MIP, zero
> > +
> > + li s7, FixedPcdGet32 (PcdHartCount) li s8, FixedPcdGet32
> > + (PcdOpenSbiStackSize)
> > +
> > + /* HART ID should be within expected limit */ csrr s6, CSR_MHARTID
> > + bge s6, s7, _start_hang
> > +
> > + /* find the scratch space for this hart */ li tp, FixedPcdGet32
> > + (PcdScratchRamBase) mul a5, s7, s8 add tp, tp, a5 mul a5, s8, s6
> > + sub tp, tp, a5 li a5, SBI_SCRATCH_SIZE sub tp, tp, a5
> > +
> > + /* update the mscratch */
> > + csrw CSR_MSCRATCH, tp
> > +
> > + /*make room for Hart specific Firmware Context*/ li a5,
> > + FIRMWARE_CONTEXT_HART_SPECIFIC_SIZE
> > + sub tp, tp, a5
> > +
> > + /* Setup stack */
> > + add sp, tp, zero
> > +
> > + /* Setup stack for the Hart executing EFI to top of temporary ram*/
> > + csrr a6, CSR_MHARTID li a5, FixedPcdGet32 (PcdBootHartId) bne a6,
> > + a5, 1f
> > +
> > + li a4, FixedPcdGet32(PcdTemporaryRamBase)
> > + li a5, FixedPcdGet32(PcdTemporaryRamSize)
> > + add sp, a4, a5
> > + 1:
> > +
> > + /* Setup trap handler */
> > + la a4, _trap_handler
> > + csrw CSR_MTVEC, a4
>
> Can you add a blank line here?
>
> > + /* Make sure that mtvec is updated */
> > + 1:
> > + csrr a5, CSR_MTVEC
> > + bne a4, a5, 1b
> > +
> > + /* Call library constructors before jup to SEC core */ call
> > + ProcessLibraryConstructorList
> > +
> > + /* jump to SEC Core C */
> > + csrr a0, CSR_MHARTID
> > + csrr a1, CSR_MSCRATCH
> > + call SecCoreStartUpWithStack
> > +
> > + /* We do not expect to reach here hence just hang */ j _start_hang
> > +
> > + .align 3
> > + .section .data, "aw"
> > +_boot_hart_done:
> > + RISCV_PTR 0
> > +
> > + .align 3
> > + .section .entry, "ax", %progbits
> > + .globl _hartid_to_scratch
> > +_hartid_to_scratch:
> > + add sp, sp, -(3 * __SIZEOF_POINTER__)
> > + sd s0, (sp)
> > + sd s1, (__SIZEOF_POINTER__)(sp)
> > + sd s2, (__SIZEOF_POINTER__ * 2)(sp)
>
> Can you add a blank line here?
>
> > + /*
> > + * a0 -> HART ID (passed by caller)
> > + * s0 -> HART Stack Size
> > + * s1 -> HART Stack End
> > + * s2 -> Temporary
> > + */
> > + la s2, platform
> > +#if __riscv_xlen == 64
> > + lwu s0, SBI_PLATFORM_HART_STACK_SIZE_OFFSET(s2)
> > + lwu s2, SBI_PLATFORM_HART_COUNT_OFFSET(s2)
> > +#else
> > + lw s0, SBI_PLATFORM_HART_STACK_SIZE_OFFSET(s2)
> > + lw s2, SBI_PLATFORM_HART_COUNT_OFFSET(s2)
> > +#endif
> > + mul s2, s2, s0
> > + li s1, FixedPcdGet32 (PcdScratchRamBase)
> > + add s1, s1, s2
> > + mul s2, s0, a0
> > + sub s1, s1, s2
> > + li s2, SBI_SCRATCH_SIZE
> > + sub a0, s1, s2
> > + ld s0, (sp)
> > + ld s1, (__SIZEOF_POINTER__)(sp)
> > + ld s2, (__SIZEOF_POINTER__ * 2)(sp)
> > + add sp, sp, (3 * __SIZEOF_POINTER__)
> > + ret
> > +
> > + .align 3
> > + .section .entry, "ax", %progbits
> > + .globl _start_hang
> > +_start_hang:
> > + wfi
> > + j _start_hang
> > +
> > + .align 3
> > + .section .entry, "ax", %progbits
> > + .globl _trap_handler
> > +_trap_handler:
> > + /* Swap TP and MSCRATCH */
> > + csrrw tp, CSR_MSCRATCH, tp
> > +
> > + /* Save T0 in scratch space */
> > + sd t0, SBI_SCRATCH_TMP0_OFFSET(tp)
> > +
> > + /* Check which mode we came from */ csrr t0, CSR_MSTATUS srl t0,
> > + t0, MSTATUS_MPP_SHIFT and t0, t0, PRV_M xori t0, t0, PRV_M beq
> > + t0, zero, _trap_handler_m_mode
> > +
> > + /* We came from S-mode or U-mode */
> > +_trap_handler_s_mode:
> > + /* Set T0 to original SP */
> > + add t0, sp, zero
> > +
> > + /* Setup exception stack */
> > + add sp, tp, -(SBI_TRAP_REGS_SIZE)
> > +
> > + /* Jump to code common for all modes */ j _trap_handler_all_mode
> > +
> > + /* We came from M-mode */
> > +_trap_handler_m_mode:
> > + /* Set T0 to original SP */
> > + add t0, sp, zero
> > +
> > + /* Re-use current SP as exception stack */ add sp, sp,
> > + -(SBI_TRAP_REGS_SIZE)
> > +
> > +_trap_handler_all_mode:
> > + /* Save original SP (from T0) on stack */
> > + sd t0, SBI_TRAP_REGS_OFFSET(sp)(sp)
> > +
> > + /* Restore T0 from scratch space */ ld t0,
> > + SBI_SCRATCH_TMP0_OFFSET(tp)
> > +
> > + /* Save T0 on stack */
> > + sd t0, SBI_TRAP_REGS_OFFSET(t0)(sp)
> > +
> > + /* Swap TP and MSCRATCH */
> > + csrrw tp, CSR_MSCRATCH, tp
> > +
> > + /* Save MEPC and MSTATUS CSRs */
> > + csrr t0, CSR_MEPC
> > + sd t0, SBI_TRAP_REGS_OFFSET(mepc)(sp) csrr t0, CSR_MSTATUS sd t0,
> > + SBI_TRAP_REGS_OFFSET(mstatus)(sp)
> > +
> > + /* Save all general regisers except SP and T0 */ sd zero,
> > + SBI_TRAP_REGS_OFFSET(zero)(sp) sd ra, SBI_TRAP_REGS_OFFSET(ra)(sp)
> > + sd gp, SBI_TRAP_REGS_OFFSET(gp)(sp) sd tp,
> > + SBI_TRAP_REGS_OFFSET(tp)(sp) sd t1, SBI_TRAP_REGS_OFFSET(t1)(sp)
> > + sd t2, SBI_TRAP_REGS_OFFSET(t2)(sp) sd s0,
> > + SBI_TRAP_REGS_OFFSET(s0)(sp) sd s1, SBI_TRAP_REGS_OFFSET(s1)(sp)
> > + sd a0, SBI_TRAP_REGS_OFFSET(a0)(sp) sd a1,
> > + SBI_TRAP_REGS_OFFSET(a1)(sp) sd a2, SBI_TRAP_REGS_OFFSET(a2)(sp)
> > + sd a3, SBI_TRAP_REGS_OFFSET(a3)(sp) sd a4,
> > + SBI_TRAP_REGS_OFFSET(a4)(sp) sd a5, SBI_TRAP_REGS_OFFSET(a5)(sp)
> > + sd a6, SBI_TRAP_REGS_OFFSET(a6)(sp) sd a7,
> > + SBI_TRAP_REGS_OFFSET(a7)(sp) sd s2, SBI_TRAP_REGS_OFFSET(s2)(sp)
> > + sd s3, SBI_TRAP_REGS_OFFSET(s3)(sp) sd s4,
> > + SBI_TRAP_REGS_OFFSET(s4)(sp) sd s5, SBI_TRAP_REGS_OFFSET(s5)(sp)
> > + sd s6, SBI_TRAP_REGS_OFFSET(s6)(sp) sd s7,
> > + SBI_TRAP_REGS_OFFSET(s7)(sp) sd s8, SBI_TRAP_REGS_OFFSET(s8)(sp)
> > + sd s9, SBI_TRAP_REGS_OFFSET(s9)(sp) sd s10,
> > + SBI_TRAP_REGS_OFFSET(s10)(sp) sd s11,
> SBI_TRAP_REGS_OFFSET(s11)(sp)
> > + sd t3, SBI_TRAP_REGS_OFFSET(t3)(sp) sd t4,
> > + SBI_TRAP_REGS_OFFSET(t4)(sp) sd t5, SBI_TRAP_REGS_OFFSET(t5)(sp)
> > + sd t6, SBI_TRAP_REGS_OFFSET(t6)(sp)
> > +
> > + /* Call C routine */
> > + add a0, sp, zero
> > + csrr a1, CSR_MSCRATCH
> > + call sbi_trap_handler
> > +
> > + /* Restore all general regisers except SP and T0 */ ld ra,
> > + SBI_TRAP_REGS_OFFSET(ra)(sp) ld gp, SBI_TRAP_REGS_OFFSET(gp)(sp)
> > + ld tp, SBI_TRAP_REGS_OFFSET(tp)(sp) ld t1,
> > + SBI_TRAP_REGS_OFFSET(t1)(sp) ld t2, SBI_TRAP_REGS_OFFSET(t2)(sp)
> > + ld s0, SBI_TRAP_REGS_OFFSET(s0)(sp) ld s1,
> > + SBI_TRAP_REGS_OFFSET(s1)(sp) ld a0, SBI_TRAP_REGS_OFFSET(a0)(sp)
> > + ld a1, SBI_TRAP_REGS_OFFSET(a1)(sp) ld a2,
> > + SBI_TRAP_REGS_OFFSET(a2)(sp) ld a3, SBI_TRAP_REGS_OFFSET(a3)(sp)
> > + ld a4, SBI_TRAP_REGS_OFFSET(a4)(sp) ld a5,
> > + SBI_TRAP_REGS_OFFSET(a5)(sp) ld a6, SBI_TRAP_REGS_OFFSET(a6)(sp)
> > + ld a7, SBI_TRAP_REGS_OFFSET(a7)(sp) ld s2,
> > + SBI_TRAP_REGS_OFFSET(s2)(sp) ld s3, SBI_TRAP_REGS_OFFSET(s3)(sp)
> > + ld s4, SBI_TRAP_REGS_OFFSET(s4)(sp) ld s5,
> > + SBI_TRAP_REGS_OFFSET(s5)(sp) ld s6, SBI_TRAP_REGS_OFFSET(s6)(sp)
> > + ld s7, SBI_TRAP_REGS_OFFSET(s7)(sp) ld s8,
> > + SBI_TRAP_REGS_OFFSET(s8)(sp) ld s9, SBI_TRAP_REGS_OFFSET(s9)(sp)
> > + ld s10, SBI_TRAP_REGS_OFFSET(s10)(sp) ld s11,
> > + SBI_TRAP_REGS_OFFSET(s11)(sp) ld t3, SBI_TRAP_REGS_OFFSET(t3)(sp)
> > + ld t4, SBI_TRAP_REGS_OFFSET(t4)(sp) ld t5,
> > + SBI_TRAP_REGS_OFFSET(t5)(sp) ld t6, SBI_TRAP_REGS_OFFSET(t6)(sp)
> > +
> > + /* Restore MEPC and MSTATUS CSRs */ ld t0,
> > + SBI_TRAP_REGS_OFFSET(mepc)(sp) csrw CSR_MEPC, t0 ld t0,
> > + SBI_TRAP_REGS_OFFSET(mstatus)(sp) csrw CSR_MSTATUS, t0
> > +
> > + /* Restore T0 */
> > + ld t0, SBI_TRAP_REGS_OFFSET(t0)(sp)
> > +
> > + /* Restore SP */
> > + ld sp, SBI_TRAP_REGS_OFFSET(sp)(sp)
> > +
> > + mret
> > +
> > + .align 3
> > + .section .entry, "ax", %progbits
> > + .globl _reset_regs
> > +_reset_regs:
> > +
> > + /* flush the instruction cache */
> > + fence.i
> > + /* Reset all registers except ra, a0,a1 */ li sp, 0 li gp, 0 li
> > + tp, 0 li t0, 0 li t1, 0 li t2, 0 li s0, 0 li s1, 0 li a2, 0
> > + li a3, 0 li a4, 0 li a5, 0 li a6, 0 li a7, 0 li s2, 0 li s3, 0
> > + li s4, 0 li s5, 0 li s6, 0 li s7, 0 li s8, 0 li s9, 0 li s10,
> > + 0 li s11, 0 li t3, 0 li t4, 0 li t5, 0 li t6, 0 csrw
> > + CSR_MSCRATCH, 0 ret
> > +
> > + .align 3
> > + .section .entry, "ax", %progbits
> > + .global fw_prev_arg1
> > +fw_prev_arg1:
> > + /* We return previous arg1 in 'a0' */
> > + add a0, zero, zero
> > + ret
> > +
> > + .align 3
> > + .section .entry, "ax", %progbits
> > + .global fw_next_arg1
> > +fw_next_arg1:
> > + /* We return next arg1 in 'a0' */
> > + li a0, FixedPcdGet32(PcdRiscVPeiFvBase)
> > + ret
> > +
> > + .align 3
> > + .section .entry, "ax", %progbits
> > + .global fw_next_addr
> > +fw_next_addr:
> > + /* We return next address in 'a0' */
> > + la a0, _jump_addr
> > + ld a0, (a0)
> > + ret
> > +
> > + .align 3
> > + .section .entry, "ax", %progbits
> > +_jump_addr:
> > + RISCV_PTR SecCoreStartUpWithStack
> > diff --git a/Platform/RiscV/Universal/Sec/SecMain.c
> > b/Platform/RiscV/Universal/Sec/SecMain.c
> > new file mode 100644
> > index 00000000..40b351ca
> > --- /dev/null
> > +++ b/Platform/RiscV/Universal/Sec/SecMain.c
> > @@ -0,0 +1,524 @@
> > +/** @file
> > + RISC-V SEC phase module.
> > +
> > + Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All
> > + rights reserved.<BR>
> > +
> > + SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "SecMain.h"
> > +#include <Library/SerialPortLib.h>
> > +#include <Library/PrintLib.h>
> > +#include <Library/DebugPrintErrorLevelLib.h>
> > +#include <sbi/sbi_hart.h>
> > +#include <sbi/sbi_scratch.h>
> > +#include <sbi/sbi_platform.h>
> > +#include <sbi/sbi.h>
> > +#include <sbi/sbi_init.h>
>
> Please sort includes alphabetically.
>
> > +#include <sbi/SbiFirmwareContext.h>
> > +
> > +int HartsIn = 0;
>
> UINTN?
>
> > +
> > +EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI mTemporaryRamSupportPpi =
> {
> > + TemporaryRamMigration
> > +};
> > +
> > +EFI_PEI_TEMPORARY_RAM_DONE_PPI mTemporaryRamDonePpi = {
> > + TemporaryRamDone
> > +};
> > +
> > +EFI_PEI_PPI_DESCRIPTOR mPrivateDispatchTable[] = {
> > + {
> > + EFI_PEI_PPI_DESCRIPTOR_PPI,
> > + &gEfiTemporaryRamSupportPpiGuid,
> > + &mTemporaryRamSupportPpi
> > + },
> > + {
> > + (EFI_PEI_PPI_DESCRIPTOR_PPI |
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
> > + &gEfiTemporaryRamDonePpiGuid,
> > + &mTemporaryRamDonePpi
> > + },
> > +};
>
> Can the above three variables all be made STATIC?
>
> > +
> > +/**
> > + Locates a section within a series of sections
> > + with the specified section type.
> > +
> > + The Instance parameter indicates which instance of the section
> > + type to return. (0 is first instance, 1 is second...)
> > +
> > + @param[in] Sections The sections to search
> > + @param[in] SizeOfSections Total size of all sections
> > + @param[in] SectionType The section type to locate
> > + @param[in] Instance The section instance number
> > + @param[out] FoundSection The FFS section if found
> > +
> > + @retval EFI_SUCCESS The file and section was found
> > + @retval EFI_NOT_FOUND The file and section was not found
> > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted
> > +
> > +**/
> > +EFI_STATUS
> > +FindFfsSectionInstance (
>
> This is a note, not something that needs to be addressed for setting up the -
> devel branch: but this function is identical to the one in
> <edk2>/OvmfPkg/Sec/SecMain.c and the one in
> Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c
> We should address this.
>
> > + IN VOID *Sections,
> > + IN UINTN SizeOfSections,
> > + IN EFI_SECTION_TYPE SectionType,
> > + IN UINTN Instance,
> > + OUT EFI_COMMON_SECTION_HEADER **FoundSection
> > + )
> > +{
> > + EFI_PHYSICAL_ADDRESS CurrentAddress;
> > + UINT32 Size;
> > + EFI_PHYSICAL_ADDRESS EndOfSections;
> > + EFI_COMMON_SECTION_HEADER *Section;
> > + EFI_PHYSICAL_ADDRESS EndOfSection;
> > +
> > + //
> > + // Loop through the FFS file sections within the PEI Core FFS file
> > + // EndOfSection = (EFI_PHYSICAL_ADDRESS)(UINTN) Sections;
> > + EndOfSections = EndOfSection + SizeOfSections; for (;;) {
> > + if (EndOfSection == EndOfSections) {
> > + break;
> > + }
> > + CurrentAddress = (EndOfSection + 3) & ~(3ULL);
> > + if (CurrentAddress >= EndOfSections) {
> > + return EFI_VOLUME_CORRUPTED;
> > + }
> > +
> > + Section = (EFI_COMMON_SECTION_HEADER*)(UINTN) CurrentAddress;
> > +
> > + Size = SECTION_SIZE (Section);
> > + if (Size < sizeof (*Section)) {
> > + return EFI_VOLUME_CORRUPTED;
> > + }
> > +
> > + EndOfSection = CurrentAddress + Size;
> > + if (EndOfSection > EndOfSections) {
> > + return EFI_VOLUME_CORRUPTED;
> > + }
> > +
> > + //
> > + // Look for the requested section type
> > + //
> > + if (Section->Type == SectionType) {
> > + if (Instance == 0) {
> > + *FoundSection = Section;
> > + return EFI_SUCCESS;
> > + } else {
> > + Instance--;
> > + }
> > + }
> > + }
> > +
> > + return EFI_NOT_FOUND;
> > +}
> > +
> > +/**
> > + Locates a section within a series of sections
> > + with the specified section type.
> > +
> > + @param[in] Sections The sections to search
> > + @param[in] SizeOfSections Total size of all sections
> > + @param[in] SectionType The section type to locate
> > + @param[out] FoundSection The FFS section if found
> > +
> > + @retval EFI_SUCCESS The file and section was found
> > + @retval EFI_NOT_FOUND The file and section was not found
> > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted
> > +
> > +**/
> > +EFI_STATUS
> > +FindFfsSectionInSections (
>
> The same applies to this one.
>
> > + IN VOID *Sections,
> > + IN UINTN SizeOfSections,
> > + IN EFI_SECTION_TYPE SectionType,
> > + OUT EFI_COMMON_SECTION_HEADER **FoundSection
> > + )
> > +{
> > + return FindFfsSectionInstance (
> > + Sections,
> > + SizeOfSections,
> > + SectionType,
> > + 0,
> > + FoundSection
> > + );
> > +}
> > +
> > +/**
> > + Locates a FFS file with the specified file type and a section
> > + within that file with the specified section type.
> > +
> > + @param[in] Fv The firmware volume to search
> > + @param[in] FileType The file type to locate
> > + @param[in] SectionType The section type to locate
> > + @param[out] FoundSection The FFS section if found
> > +
> > + @retval EFI_SUCCESS The file and section was found
> > + @retval EFI_NOT_FOUND The file and section was not found
> > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted
> > +
> > +**/
> > +EFI_STATUS
> > +FindFfsFileAndSection (
>
> And this one.
>
> > + IN EFI_FIRMWARE_VOLUME_HEADER *Fv,
> > + IN EFI_FV_FILETYPE FileType,
> > + IN EFI_SECTION_TYPE SectionType,
> > + OUT EFI_COMMON_SECTION_HEADER **FoundSection
> > + )
> > +{
> > + EFI_STATUS Status;
> > + EFI_PHYSICAL_ADDRESS CurrentAddress;
> > + EFI_PHYSICAL_ADDRESS EndOfFirmwareVolume;
> > + EFI_FFS_FILE_HEADER *File;
> > + UINT32 Size;
> > + EFI_PHYSICAL_ADDRESS EndOfFile;
> > +
> > + if (Fv->Signature != EFI_FVH_SIGNATURE) {
> > + DEBUG ((DEBUG_ERROR, "FV at %p does not have FV header
> signature\n", Fv));
> > + return EFI_VOLUME_CORRUPTED;
> > + }
> > +
> > + CurrentAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Fv;
> > + EndOfFirmwareVolume = CurrentAddress + Fv->FvLength;
> > +
> > + //
> > + // Loop through the FFS files in the Boot Firmware Volume // for
> > + (EndOfFile = CurrentAddress + Fv->HeaderLength; ; ) {
> > +
> > + CurrentAddress = (EndOfFile + 7) & ~(7ULL);
> > + if (CurrentAddress > EndOfFirmwareVolume) {
> > + return EFI_VOLUME_CORRUPTED;
> > + }
> > +
> > + File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress;
> > + Size = *(UINT32*) File->Size & 0xffffff;
> > + if (Size < (sizeof (*File) + sizeof (EFI_COMMON_SECTION_HEADER))) {
> > + return EFI_VOLUME_CORRUPTED;
> > + }
> > +
> > + EndOfFile = CurrentAddress + Size;
> > + if (EndOfFile > EndOfFirmwareVolume) {
> > + return EFI_VOLUME_CORRUPTED;
> > + }
> > +
> > + //
> > + // Look for the request file type
> > + //
> > + if (File->Type != FileType) {
> > + continue;
> > + }
> > +
> > + Status = FindFfsSectionInSections (
> > + (VOID*) (File + 1),
> > + (UINTN) EndOfFile - (UINTN) (File + 1),
> > + SectionType,
> > + FoundSection
> > + );
> > + if (!EFI_ERROR (Status) || (Status == EFI_VOLUME_CORRUPTED)) {
> > + return Status;
> > + }
> > + }
> > +}
> > +
> > +/**
> > + Locates the PEI Core entry point address
> > +
> > + @param[in] Fv The firmware volume to search
> > + @param[out] PeiCoreEntryPoint The entry point of the PEI Core
> > + image
> > +
> > + @retval EFI_SUCCESS The file and section was found
> > + @retval EFI_NOT_FOUND The file and section was not found
> > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted
> > +
> > +**/
> > +EFI_STATUS
> > +FindPeiCoreImageBaseInFv (
>
> And this.
>
> > + IN EFI_FIRMWARE_VOLUME_HEADER *Fv,
> > + OUT EFI_PHYSICAL_ADDRESS *PeiCoreImageBase
> > + )
> > +{
> > + EFI_STATUS Status;
> > + EFI_COMMON_SECTION_HEADER *Section;
> > +
> > + Status = FindFfsFileAndSection (
> > + Fv,
> > + EFI_FV_FILETYPE_PEI_CORE,
> > + EFI_SECTION_PE32,
> > + &Section
> > + );
> > + if (EFI_ERROR (Status)) {
> > + Status = FindFfsFileAndSection (
> > + Fv,
> > + EFI_FV_FILETYPE_PEI_CORE,
> > + EFI_SECTION_TE,
> > + &Section
> > + );
> > + if (EFI_ERROR (Status)) {
> > + DEBUG ((DEBUG_ERROR, "Unable to find PEI Core image\n"));
> > + return Status;
> > + }
> > + }
> > + DEBUG ((DEBUG_ERROR, "PeiCoreImageBase found\n"));
> > + *PeiCoreImageBase = (EFI_PHYSICAL_ADDRESS)(UINTN)(Section + 1);
> > + return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + Locates the PEI Core entry point address
> > +
> > + @param[in,out] Fv The firmware volume to search
> > + @param[out] PeiCoreEntryPoint The entry point of the PEI Core image
> > +
> > + @retval EFI_SUCCESS The file and section was found
> > + @retval EFI_NOT_FOUND The file and section was not found
> > + @retval EFI_VOLUME_CORRUPTED The firmware volume was corrupted
> > +
> > +**/
> > +VOID
> > +FindPeiCoreImageBase (
> > + IN OUT EFI_FIRMWARE_VOLUME_HEADER **BootFv,
> > + OUT EFI_PHYSICAL_ADDRESS *PeiCoreImageBase
> > + )
> > +{
> > + *PeiCoreImageBase = 0;
> > +
> > + DEBUG ((DEBUG_INFO, "FindPeiCoreImageBaseInFv\n"));
>
> This one is the same with S3 support ripped out, and the above line added.
> Can you delete the message, or change it to something more human friendly?
> I personally would interpret that as something that was printed _from_ that
> function (and in that situation should be using %a __FUNCTION__).
>
> > + FindPeiCoreImageBaseInFv (*BootFv, PeiCoreImageBase); }
> > +
> > +/*
> > + Find and return Pei Core entry point.
> > +
> > + It also find SEC and PEI Core file debug inforamtion. It will
> > + report them if remote debug is enabled.
> > +
> > +**/
> > +VOID
> > +FindAndReportEntryPoints (
> > + IN EFI_FIRMWARE_VOLUME_HEADER **BootFirmwareVolumePtr,
> > + OUT EFI_PEI_CORE_ENTRY_POINT *PeiCoreEntryPoint
> > + )
> > +{
> > + EFI_STATUS Status;
> > + EFI_PHYSICAL_ADDRESS PeiCoreImageBase;
> > +
> > + DEBUG ((DEBUG_INFO, "FindAndReportEntryPoints\n"));
>
> Use %a __FUNCTION__.
>
> > +
> > + FindPeiCoreImageBase (BootFirmwareVolumePtr, &PeiCoreImageBase);
> > + // // Find PEI Core entry point // Status =
> > + PeCoffLoaderGetEntryPoint ((VOID *) (UINTN) PeiCoreImageBase,
> > + (VOID**) PeiCoreEntryPoint); if (EFI_ERROR(Status)) {
> > + *PeiCoreEntryPoint = 0;
> > + }
> > + DEBUG ((DEBUG_INFO, "PeCoffLoaderGetEntryPoint success: %x\n",
> > + *PeiCoreEntryPoint));
> > +
> > + return;
> > +}
> > +/*
> > + Print out the content of firmware context.
> > +
> > +**/
> > +VOID
> > +DebutPrintFirmwareContext (
>
> Debut -> Debug?
>
> > + EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *FirmwareContext
> > + )
> > +{
> > + DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Firmware Context at
> > +0x%x\n", FirmwareContext));
>
> Please drop the [OpenSBI] prefix.
>
> > + DEBUG ((DEBUG_INFO, " PEI Service at 0x%x\n\n",
> FirmwareContext->PeiServiceTable));
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +TemporaryRamMigration (
> > + IN CONST EFI_PEI_SERVICES **PeiServices,
> > + IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
> > + IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
> > + IN UINTN CopySize
> > + )
> > +{
> > + VOID *OldHeap;
> > + VOID *NewHeap;
> > + VOID *OldStack;
> > + VOID *NewStack;
> > + struct sbi_platform *ThisSbiPlatform;
> > +
> > + DEBUG ((DEBUG_INFO,
> > + "TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
>
> %a
> __FUNCTION__
>
> > + TemporaryMemoryBase,
> > + PermanentMemoryBase,
> > + (UINT64)CopySize
> > + ));
> > +
> > + OldHeap = (VOID*)(UINTN)TemporaryMemoryBase;
> > + NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1));
> > +
> > + OldStack = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1));
> > + NewStack = (VOID*)(UINTN)PermanentMemoryBase;
> > +
> > + CopyMem (NewHeap, OldHeap, CopySize >> 1); // Migrate Heap
> > + CopyMem (NewStack, OldStack, CopySize >> 1); // Migrate Stack
> > +
> > + //
> > + // Reset firmware context pointer
> > + //
> > + ThisSbiPlatform = (struct sbi_platform
> > + *)sbi_platform_ptr(sbi_scratch_thishart_ptr());
> > + ThisSbiPlatform->firmware_context += (unsigned
> > + long)((UINTN)NewStack - (UINTN)OldStack);
>
> "unsigned long" is not a type that exists in UEFI.
> Please use UINTN. Applies throughout.
>
> > + //
> > + // Relocate PEI Service **
> > + //
> > + ((EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT
> > + *)ThisSbiPlatform->firmware_context)->PeiServiceTable += (unsigned
> > + long)((UINTN)NewStack - (UINTN)OldStack);
>
> The above line is begging for a temporary pointer for the firmware_context.
>
> > + DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Firmware Context is
> > + relocated to 0x%x\n", ThisSbiPlatform->firmware_context));
>
> Please
>
> > + DebutPrintFirmwareContext
> ((EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT
> > + *)ThisSbiPlatform->firmware_context);
> > +
> > + register uintptr_t a0 asm ("a0") = (uintptr_t)((UINTN)NewStack -
> > + (UINTN)OldStack); asm volatile ("add sp, sp, a0"::"r"(a0):);
>
> Urgh.
> Is there any way this could be broken out into a separate assembler function
> instead? If not, we still need to get rid of the uintptr_t:s.
>
> > + return EFI_SUCCESS;
> > +}
> > +
> > +EFI_STATUS EFIAPI TemporaryRamDone (VOID) {
> > + DEBUG ((DEBUG_INFO, "2nd time PEI core, temporary ram done.\n"));
> > + return EFI_SUCCESS;
> > +}
> > +
> > +#if 1
>
> Just delete the #if 1?
>
> > +#define GPIO_CTRL_ADDR 0x54002000
> > +#define GPIO_OUTPUT_VAL 0x0C
> > +static volatile UINT32 * const gpio = (void *)GPIO_CTRL_ADDR;
> > +#define REG32(p, i) ((p)[(i)>>2])
> > +#endif
>
> And the #endif
>
> Moreover, please move the ADDRESS/VALUE macros to SecMain.h, and use
> IoLib instead of defining your own variables and macros.
>
> > +
> > +static VOID EFIAPI PeiCore(VOID)
>
> STATIC
> Also, please follow coding style for function definition, this is supposed to
> be
> over multiple lines.
>
> > +{
> > + EFI_SEC_PEI_HAND_OFF SecCoreData;
> > + EFI_PEI_CORE_ENTRY_POINT PeiCoreEntryPoint;
> > + EFI_FIRMWARE_VOLUME_HEADER *BootFv =
> (EFI_FIRMWARE_VOLUME_HEADER
> > +*)FixedPcdGet32(PcdRiscVPeiFvBase);
> > + EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT FirmwareContext;
> > + struct sbi_platform *ThisSbiPlatform;
> > + UINT32 HartId;
> > +
> > + REG32(gpio, GPIO_OUTPUT_VAL) = 0x88; FindAndReportEntryPoints
> > + (&BootFv, &PeiCoreEntryPoint);
> > +
> > + SecCoreData.DataSize = sizeof(EFI_SEC_PEI_HAND_OFF);
> > + SecCoreData.BootFirmwareVolumeBase = BootFv;
> > + SecCoreData.BootFirmwareVolumeSize = (UINTN) BootFv->FvLength;
> > + SecCoreData.TemporaryRamBase = (VOID*)(UINT64)
> FixedPcdGet32(PcdTemporaryRamBase);
> > + SecCoreData.TemporaryRamSize = (UINTN)
> FixedPcdGet32(PcdTemporaryRamSize);
> > + SecCoreData.PeiTemporaryRamBase =
> SecCoreData.TemporaryRamBase;
> > + SecCoreData.PeiTemporaryRamSize = SecCoreData.TemporaryRamSize
> >> 1;
> > + SecCoreData.StackBase = (UINT8
> *)SecCoreData.TemporaryRamBase + (SecCoreData.TemporaryRamSize >>
> 1);
>
> Please don't use UINT8 * to get away from pointer arithmetic. It disguises
> what function is actually being performed.
> (VOID *)((UINTN) ...) is a much preferred form where that level of
> description is required.
>
> > + SecCoreData.StackSize = SecCoreData.TemporaryRamSize >> 1;
> > +
> > + //
> > + // Print out scratch address of each hart // DEBUG ((DEBUG_INFO,
> > + "[OpenSBI]: OpenSBI scratch address for each hart:\n"));
>
> Please drop [OpenSBI] tag.
>
> > + for (HartId = 0; HartId < FixedPcdGet32 (PcdHartCount); HartId ++) {
> > + DEBUG ((DEBUG_INFO, " Hart %d: 0x%x\n", HartId,
> sbi_hart_id_to_scratch(sbi_scratch_thishart_ptr(), HartId)));
>
> Please wrap long line.
>
> > + }
> > +
> > + //
> > + // Set up OpepSBI firmware context poitner on boot hart OpenSbi
> > + scratch. Firmware context residents in stack and will be
>
> Please wrap long line.
>
> > + // switched to memory when temporary ram migration.
> > + //
> > + ZeroMem ((VOID *)&FirmwareContext, sizeof
> > + (EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT));
> > + ThisSbiPlatform = (struct sbi_platform
> > + *)sbi_platform_ptr(sbi_scratch_thishart_ptr());
> > + if (ThisSbiPlatform->opensbi_version > OPENSBI_VERSION) {
> > + DEBUG ((DEBUG_ERROR, "[OpenSBI]: OpenSBI platform table version
> 0x%x is newer than OpenSBI version 0x%x.\n"
> > + "There maybe be some backward compatable
> > + issues.\n",
>
> Please drop [OpenSBI] tag. Please convert the two lines to two separate
> DEBUG prints.
>
> > + ThisSbiPlatform->opensbi_version,
> > + OPENSBI_VERSION
> > + ));
> > + ASSERT(FALSE);
> > + }
> > + DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI platform table at address:
> > + 0x%x\nFirmware Context is located at 0x%x\n",
>
> Please drop [OpenSBI] tag.
>
> > + ThisSbiPlatform,
> > + &FirmwareContext
> > + ));
> > + ThisSbiPlatform->firmware_context = (unsigned
> > + long)&FirmwareContext;
>
> UINTN (probably best to do a global search and replace on this).
>
> > + //
> > + // Set firmware context Hart-specific pointer // for (HartId = 0;
> > + HartId < FixedPcdGet32 (PcdHartCount); HartId ++) {
> > + FirmwareContext.HartSpecific [HartId] = \
> > + (EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC *)((UINT8
> > + *)sbi_hart_id_to_scratch(sbi_scratch_thishart_ptr(), HartId) -
> > + FIRMWARE_CONTEXT_HART_SPECIFIC_SIZE);
>
> Please don't use (UINT8 *) to get away from pointer arithmetic.
> UINTN should work fine in this context.
>
> Screaming out for a temporary variable.
>
> > + DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Hart %d Firmware
> Context
> > + Hart-specific at address: 0x%x\n",
>
> Please drop [OpenSBI] tag.
>
> > + HartId,
> > + FirmwareContext.HartSpecific [HartId]
> > + ));
> > + }
> > +
> > + //
> > + // Transfer the control to the PEI core
> > + //
> > + (*PeiCoreEntryPoint) (&SecCoreData, (EFI_PEI_PPI_DESCRIPTOR
> > +*)&mPrivateDispatchTable); }
> > +/**
> > + This function initilizes hart specific information and SBI.
> > + For the boot hart, it boots system through PEI core and initial SBI in
> > the
> DXE IPL.
> > + For others, it goes to initial SBI and halt.
> > +
> > + the lay out of memory region for each hart is as below delineates,
> > +
> > + _
> > ____
> > + |----Scratch ends |
> > |
> > + | | sizeof (sbi_scratch)
> > |
> > + | _|
> > |
> > + |----Scratch buffer start s <----- *scratch
> > |
> > + |----Firmware Context Hart-specific ends _
> > |
> > + | |
> > |
> > + | |
> > FIRMWARE_CONTEXT_HART_SPECIFIC_SIZE
> |
> > + | |
> > | PcdOpenSbiStackSize
> > + | _|
> > |
> > + |----Firmware Context Hart-specific starts <-----
> **HartFirmwareContext |
> > + |----Hart stack top _
> > |
> > + | |
> > |
> > + | |
> > |
> > + | | Stack
> > |
> > + | |
> > |
> > + | _|
> > ____|
> > + |----Hart stack bottom
> > +
> > +**/
> > +VOID EFIAPI SecCoreStartUpWithStack(unsigned long hartid, struct
> > +sbi_scratch *scratch)
>
> Split up over multiple lines.
>
> > +{
> > + EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC
> *HartFirmwareContext;
> > +
> > + //
> > + // Setup EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC for each hart.
> > + //
> > + HartFirmwareContext =
> (EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC
> > + *)((UINT8 *)scratch - FIRMWARE_CONTEXT_HART_SPECIFIC_SIZE);
>
> Please don't use UINT8 * to get away from pointer arithmetic.
> In this context, UINTN should work just fine.
>
> > + HartFirmwareContext->IsaExtensionSupported = RiscVReadMisa ();
> > + HartFirmwareContext->MachineVendorId.Value64_L =
> RiscVReadMVendorId
> > + (); HartFirmwareContext->MachineVendorId.Value64_H = 0;
> > + HartFirmwareContext->MachineArchId.Value64_L = RiscVReadMArchId
> ();
> > + HartFirmwareContext->MachineArchId.Value64_H = 0;
> > + HartFirmwareContext->MachineImplId.Value64_L = RiscVReadMImplId ();
> > + HartFirmwareContext->MachineImplId.Value64_H = 0;
> > +
> > +#if 0
>
> Please don't leave unused code in.
> If you want to keep the printouts for extreme debugging sessions, change
> the loglevel to DEBUG_VERBOSE.
>
> > + while (HartsIn != hartid);
> > + DEBUG ((DEBUG_INFO, "[OpenSBI]: Initial Firmware Context
> > + Hart-specific for HART ID:%d\n", hartid));
>
> Please drop [OpenSBI] tag.
>
> > + DEBUG ((DEBUG_INFO, " Scratch at address: 0x%x\n", scratch));
> > + DEBUG ((DEBUG_INFO, " Firmware Context Hart-specific at
> > address:
> 0x%x\n", HartFirmwareContext));
> > + DEBUG ((DEBUG_INFO, " stack pointer at address: 0x%x\n",
> stack_point));
> > + DEBUG ((DEBUG_INFO, " MISA: 0x%x\n",
> HartFirmwareContext->IsaExtensionSupported));
> > + DEBUG ((DEBUG_INFO, " MVENDORID: 0x%x\n",
> HartFirmwareContext->MachineVendorId.Value64_L));
> > + DEBUG ((DEBUG_INFO, " MARCHID: 0x%x\n",
> HartFirmwareContext->MachineArchId.Value64_L));
> > + DEBUG ((DEBUG_INFO, " MIMPID: 0x%x\n\n",
> HartFirmwareContext->MachineImplId.Value64_L));
> > + HartsIn ++;
> > + for (;;);
> > +#endif
> > +
> > + if (hartid == FixedPcdGet32(PcdBootHartId)) {
> > + PeiCore();
> > + }
> > + sbi_init(scratch);
> > +}
> > diff --git a/Platform/RiscV/Universal/Sec/SecMain.h
> > b/Platform/RiscV/Universal/Sec/SecMain.h
> > new file mode 100644
> > index 00000000..e7565f5e
> > --- /dev/null
> > +++ b/Platform/RiscV/Universal/Sec/SecMain.h
> > @@ -0,0 +1,57 @@
> > +/** @file
> > + RISC-V SEC phase module definitions..
> > +
> > + Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All
> > + rights reserved.<BR>
> > +
> > + SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef _SECMAIN_H_
> > +#define _SECMAIN_H_
>
> Plese drop leading _.
>
> > +
> > +#include <PiPei.h>
> > +#include <Library/PeimEntryPoint.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/PeiServicesLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/DebugAgentLib.h>
> > +#include <Library/IoLib.h>
> > +#include <Library/PeCoffLib.h>
> > +#include <Library/PeCoffGetEntryPointLib.h>
> > +#include <Library/PeCoffExtraActionLib.h> #include
> > +<Library/ExtractGuidedSectionLib.h>
> > +#include <Library/HobLib.h>
> > +#include <Ppi/TemporaryRamSupport.h>
> > +#include <Ppi/TemporaryRamDone.h>
> > +#include <Library/RiscVCpuLib.h>
>
> Please drop all include statements not required by this file, and add them
> instead in the files that do use them.
>
> > +
> > +VOID
> > +SecMachineModeTrapHandler (
> > + IN VOID
> > + );
> > +
> > +VOID
> > +EFIAPI
> > +SecStartupPhase2 (
> > + IN VOID *Context
> > + );
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +TemporaryRamMigration (
> > + IN CONST EFI_PEI_SERVICES **PeiServices,
> > + IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
> > + IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
> > + IN UINTN CopySize
> > + );
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +TemporaryRamDone (
> > + VOID
> > + );
> > +
> > +#endif // _SECMAIN_H_
> > diff --git a/Platform/RiscV/Universal/Sec/SecMain.inf
> > b/Platform/RiscV/Universal/Sec/SecMain.inf
> > new file mode 100644
> > index 00000000..c408fc8d
> > --- /dev/null
> > +++ b/Platform/RiscV/Universal/Sec/SecMain.inf
> > @@ -0,0 +1,75 @@
> > +## @file
> > +# RISC-V SEC module.
> > +#
> > +# Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All
> > +rights reserved.<BR> # # SPDX-License-Identifier:
> > +BSD-2-Clause-Patent # ##
> > +
> > +[Defines]
> > + INF_VERSION = 0x00010005
>
> Can this be updated to a recent specification version.
>
> > + BASE_NAME = SecMain
> > + FILE_GUID = df1ccef6-f301-4a63-9661-fc6030dcc880
> > + MODULE_TYPE = SEC
> > + VERSION_STRING = 1.0
> > + ENTRY_POINT = SecMain
> > +
> > +#
> > +# The following information is for reference only and not required by the
> build tools.
> > +#
> > +# VALID_ARCHITECTURES = RISCV64 EBC
>
> Pretty sure EBC is not valid here.
>
> > +#
> > +
> > +[Sources]
> > + SecMain.c
> > +
> > +[Sources.RISCV64]
> > + Riscv64/SecEntry.s
> > +
> > +[Packages]
> > + MdePkg/MdePkg.dec
> > + MdeModulePkg/MdeModulePkg.dec
> > + RiscVPkg/RiscVPkg.dec
> > + Platform/RiscV/RiscVPlatformPkg.dec
> > +
> > +[LibraryClasses]
> > + BaseLib
> > + DebugLib
> > + BaseMemoryLib
> > + PcdLib
> > + DebugAgentLib
> > + IoLib
> > + PeCoffLib
> > + PeCoffGetEntryPointLib
> > + PeCoffExtraActionLib
> > + ExtractGuidedSectionLib
> > + RiscVCpuLib
> > + PrintLib
> > + SerialPortLib
> > + RiscVOpensbiLib
> > + OpenSbiPlatformLib # This is required library which
> > + # provides platform level opensbi
> > + # functions.
>
> I don't doubt it, but I can tell it's required from the fact that it is
> listed. And
> the name itself says all of the rest. The comment is superfluous and can be
> deleted.
>
> > +
> > +[Ppis]
> > + gEfiTemporaryRamSupportPpiGuid # PPI ALWAYS_PRODUCED
> > + gEfiTemporaryRamDonePpiGuid # PPI ALWAYS_PRODUCED
> > +
> > +[Guids]
> > + gUefiRiscVMachineContextGuid
> > +
> > +[FixedPcd]
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdRiscVPeiFvBase
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdRiscVPeiFvSize
> > +
> > +[Pcd]
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdFwStartAddress
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdFwEndAddress
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdHartCount
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdBootHartId
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdScratchRamBase
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdScratchRamSize
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdOpenSbiStackSize
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdTemporaryRamBase
> > + gUefiRiscVPlatformPkgTokenSpaceGuid.PcdTemporaryRamSize
>
> Please sort the above alphabetically (
> apart from where it makes sense not to - for example PcdFwStartAddress
> before PcdFwEndAddress makes more sense than the other way around).
>
> /
> Leif
>
> > --
> > 2.12.0.windows.1
> >
> >
> >
> >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49014): https://edk2.groups.io/g/devel/message/49014
Mute This Topic: https://groups.io/mt/34196353/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
