Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 06/14] RiscV/Universal: Initial version of common RISC-V SEC module

["Leif Lindholm" <leif.lindholm@linaro.org>]

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.

> Signed-off-by: Gilbert Chen <gilbert.chen@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.patel@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
> 
> 
> 
>