Add SiFive U500 VC707 FPGA Platform

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

Posted by Gilbert Chen 6 years, 4 months ago

Common RISC-V SEC module for RISC-V platforms.

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>
+ *
+ */
+
+#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
+
+  // 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)
+  /* Note: fw_next_arg1() uses a0, a1, and ra */
+  call fw_next_arg1
+  sd a0, SBI_SCRATCH_NEXT_ARG1_OFFSET(tp)
+  /* Note: fw_next_addr() uses a0, a1, and ra */
+  call fw_next_addr
+  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)
+#ifdef FW_OPTIONS
+  li a4, FW_OPTIONS
+  sd a4, SBI_SCRATCH_OPTIONS_OFFSET(tp)
+#else
+  sd zero, SBI_SCRATCH_OPTIONS_OFFSET(tp)
+#endif
+  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
+  /* 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)
+  /*
+   * 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>
+#include <sbi/SbiFirmwareContext.h>
+
+int HartsIn = 0;
+
+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
+  },
+};
+
+/**
+  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 (
+  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 (
+  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 (
+  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 (
+  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"));
+  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"));
+
+  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 (
+    EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *FirmwareContext
+    )
+{
+  DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Firmware Context at 0x%x\n", FirmwareContext));
+  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",
+    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);
+  //
+  // Relocate PEI Service **
+  //
+  ((EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT *)ThisSbiPlatform->firmware_context)->PeiServiceTable += (unsigned long)((UINTN)NewStack - (UINTN)OldStack);
+  DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Firmware Context is relocated to 0x%x\n", ThisSbiPlatform->firmware_context));
+  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):);
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS EFIAPI TemporaryRamDone (VOID)
+{
+  DEBUG ((DEBUG_INFO, "2nd time PEI core, temporary ram done.\n"));
+  return EFI_SUCCESS;
+}
+
+#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
+
+static VOID EFIAPI PeiCore(VOID)
+{
+  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);
+  SecCoreData.StackSize              = SecCoreData.TemporaryRamSize >> 1;
+
+  //
+  // Print out scratch address of each hart
+  //
+  DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI scratch address for each hart:\n"));
+  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)));
+  }
+
+  //
+  // Set up OpepSBI firmware context poitner on boot hart OpenSbi scratch. Firmware context residents in stack and will be
+  // 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",
+             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",
+             ThisSbiPlatform,
+             &FirmwareContext
+             ));
+  ThisSbiPlatform->firmware_context = (unsigned long)&FirmwareContext;
+  //
+  // 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);
+    DEBUG ((DEBUG_INFO, "[OpenSBI]: OpenSBI Hart %d Firmware Context Hart-specific at address: 0x%x\n",
+             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)
+{
+  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);
+  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
+  while (HartsIn != hartid);
+  DEBUG ((DEBUG_INFO, "[OpenSBI]: Initial Firmware Context Hart-specific for HART ID:%d\n", hartid));
+  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_
+
+#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>
+
+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
+  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
+#
+
+[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.
+
+[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
-- 
2.12.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47539): https://edk2.groups.io/g/devel/message/47539
Mute This Topic: https://groups.io/mt/34196353/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Leif Lindholm 6 years, 4 months ago

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
> 
> 
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48392): https://edk2.groups.io/g/devel/message/48392
Mute This Topic: https://groups.io/mt/34196353/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Abner Chang 6 years, 3 months ago


> -----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.chen@hpe.com>
> Cc: Palmer Dabbelt <palmer@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.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
> >
> >
> >
> >
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-